Test RemoteDandiset.refresh() #740
Conversation
Codecov Report
@@ Coverage Diff @@
## master #740 +/- ##
==========================================
+ Coverage 84.68% 84.84% +0.15%
==========================================
Files 59 59
Lines 5962 5977 +15
==========================================
+ Hits 5049 5071 +22
+ Misses 913 906 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -425,6 +425,7 @@ def version(self) -> Version: | |||
| self._version_id is None or vdict["version"] == self.version_id | |||
| ): | |||
| self._version = Version.parse_obj(vdict) | |||
| self._version_id = self._version.identifier | |||
There was a problem hiding this comment.
hm, so any time we change _version , _version_id should be (re)set as well? here is what I see ATM:
(git)lena:~/proj/dandi/dandi-cli-master[master]git
$> git grep -p -2 'self._version = '
dandi/dandiapi.py=class RemoteDandiset:
--
dandi/dandiapi.py- if version is None:
dandi/dandiapi.py- self._version_id = None
dandi/dandiapi.py: self._version = None
dandi/dandiapi.py- elif isinstance(version, str):
dandi/dandiapi.py- self._version_id = version
dandi/dandiapi.py: self._version = None
dandi/dandiapi.py- else:
dandi/dandiapi.py- self._version_id = version.identifier
dandi/dandiapi.py: self._version = version
dandi/dandiapi.py- self._data = data
dandi/dandiapi.py-
--
dandi/dandiapi.py- self._version_id is None or vdict["version"] == self.version_id
dandi/dandiapi.py- ):
dandi/dandiapi.py: self._version = Version.parse_obj(vdict)
dandi/dandiapi.py- return self._version
dandi/dandiapi.py- assert self._version_id is not None
dandi/dandiapi.py: self._version = self.get_version(self._version_id)
dandi/dandiapi.py- return self._version
dandi/dandiapi.py-
--
dandi/dandiapi.py- self._data = self.client.get(f"/dandisets/{self.identifier}/")
dandi/dandiapi.py- # Clear _version so it will be refetched the next time it is accessed
dandi/dandiapi.py: self._version = None
dandi/dandiapi.py-
dandi/dandiapi.py- def get_versions(self) -> Iterator[Version]:
--
dandi/dandiapi.py- self.client.delete(self.api_path)
dandi/dandiapi.py- self._data = None
dandi/dandiapi.py: self._version = None
dandi/dandiapi.py-
dandi/dandiapi.py- def get_metadata(self) -> models.Dandiset:
so we seems to do that tandem almost all the time -- should it be done all the time? if so - may be _version itself should become a property and do set __version with actual Version object and also (re)set _version_id while doing the change?
There was a problem hiding this comment.
The new line covers the case where the version is None when the RemoteDandiset is created; if refresh() is then called without accessing version_id first, then, without this line, the RemoteDandiset's version could get updated to a newer version than existed when the object was created. This line sets version_id as soon as the version is first accessed so that the RemoteDandiset will continue pointing to the same version even after a refresh. There's still the possibility for the instance's version to change if a RemoteDandiset is refreshed without accessing any version data, but that can't really be worked around for lazy instances without becoming non-lazy.
I don't see a point to your property suggestion.
There was a problem hiding this comment.
is is generally true or false that "we need to set ._version_id any time we change ._version"?
There was a problem hiding this comment.
It is not true. After initialization of a RemoteDandiset, the only times that _version should be modified are (a) when accessing version lazily for the first time, in which case _version_id only needs to be set if it is None, (b) when calling refresh(), in which case _version_id needs to remain as-is, and (c) when calling delete(), in which case it doesn't really matter what happens to _version_id.
There was a problem hiding this comment.
well, the question was not "when do we need to set _version", but rather about how tight coupling between _version_id and _version, and either that logic ideally shouldn't be put together in single piece of code instead of spread around the code base.
But ok, let's proceed as is for now, but I guess the same question might come up again whenever I see them modified in tandem or forgotten to be modified in tandem.
Closes #739.