daf_persistence Tickets/dm 9039 #43
Conversation
n8pease
force-pushed
the
tickets/DM-9039
branch
from
c99d611
to
e01a06c
Compare
handler function for creating inputs & outputs when Butler.__init__ is called with root, mapper, mapperArgs
maybe just for now
also change most locations that abbreviated repository to repo to use full word "repository"
IE make it official. Moves the arg into the repocfg constructor, and adjusts use cases. Allows setting mapper in cfg if the cfg is for a new repo.
they are related to Butler state and don't belong in an object that gets serialized.
(said 'read only', clearly meant 'write only'; outputs by default are write only, which is what is done here.)
The new Butler init sequence requires that input repositories exist. This test was using an input that did not exist, and it did not matter that the repo did not exist, because the test is entirely for processing mapper data, that is hard coded into the test mapper for this test. Fix is to make the the repo used by the Butler for this test be an output with mode='rw' instead of an input with mode='r'.
Butler now handles v1 repos in _createRepoData
some of the unit tests use a folder called _parent (instead of a symlink called _parent to another folder).
n8pease
force-pushed
the
tickets/DM-9039
branch
from
e01a06c
to
dccaae3
Compare
tests/repositoryArgs.py
Outdated
| @@ -115,6 +125,8 @@ def testAbsoluteFilePathWithSchema(self): | |||
| butler = dp.Butler(outputs=args) | |||
| self.assertTrue(os.path.exists(os.path.join(self.testDir, 'repositoryCfg.yaml'))) | |||
|
|
|||
| butler2 = dp.Butler(inputs=uri) | |||
There was a problem hiding this comment.
Do you mean to perform the same exact call twice?
It seems to me you only want the second call on line 131 which explicitly fails the test if an exception is thrown
| repo1Root = os.path.join(self.testDir, 'repo1') | ||
| repo2Root = os.path.join(self.testDir, 'repo2') | ||
| butler = dp.Butler(outputs=(dp.RepositoryArgs(root=repo1Root, mapper=mapper1))) | ||
| del butler |
There was a problem hiding this comment.
sometimes it clarifies intention (a new one is created later), and other times it gets the linter to stop complaining about unused variables. and sometimes both.
| """Test if a URI is for a local filesystem storage. | ||
|
|
||
| This is mostly for backward compatibility; Butler V1 repositories were only ever on the local | ||
| filesystem. They may exist but not have a RepositoryCfg class. This enables conditional checking for a |
| True if the URI is associated with a posix storage, else false. | ||
| """ | ||
| parseRes = urllib.parse.urlparse(uri) | ||
| if parseRes.scheme in ('file', ''): |
There was a problem hiding this comment.
Maybe add to the function documentation that you are checking for the explicit file scheme in the uri or interpreting the lack of a specific scheme to tacitly imply thefile scheme. I say this just because this line looks quite funny at first glance - it's not apparent that you are accepting the lack of a scheme at first glance.
| @@ -179,6 +200,7 @@ def map(self, *args, **kwargs): | |||
| :return: The type of item is dependent on the mapper being used but is typically a ButlerLocation. | |||
| """ | |||
| if self._mapper is None: | |||
| raise RuntimeError("no mapper for repo") # remind me why we allow this | |||
| def cfgRoot(self): | ||
| if self._cfgRoot is not None: | ||
| return self._cfgRoot | ||
| else: | ||
| return self.root | ||
|
|
||
| @property | ||
| def root(self): | ||
| if self._root is not None: |
There was a problem hiding this comment.
Is it possible for self._root or self._cfgRoot to ever be False?
If not, potentially consider:
return self._root if self._root else self._cfgRoot
(and the opposite for def cfgRoot(self): above
| """ | ||
| if not os.path.exists(root): | ||
| return False | ||
| return os.listdir(root) |
There was a problem hiding this comment.
os.listdir returns a list. len(os.listdir)?
Maybe:
return os.path.exists(root) and len(os.listdir(root))
There was a problem hiding this comment.
actually:
return os.path.exists(root) and bool(os.listdir(root))
but yup. ok.
| isV1Args = inputs is None and outputs is None | ||
| if isV1Args: | ||
| inputs, outputs = self._convertV1Args(root=root, mapper=mapper, mapperArgs=mapperArgs) | ||
| elif root: |
There was a problem hiding this comment.
If root is throwing a RuntimeError, shouldn't the message say "root is no longer a supported parameter", since deprecated usually means warning.
| return parents | ||
|
|
||
| def _createRepoDatas(self, inputs, outputs): | ||
| """Create the RepoDataContainer and put a RepoData object in it for each repository listed in inputs |
The test used to rely on mapper checking before trying to load repos. The Butler has changed so that this no longer works, fix the test to actually create repos.
must have an input repo available to use it as a read-only repo.
n8pease
force-pushed
the
tickets/DM-9039
branch
from
2a9bb4e
to
718f58e
Compare
No description provided.