DM-23275: Add back makeRepo support for outfile #227
Conversation
python/lsst/daf/butler/_butler.py
Outdated
| # the root of the butler repo in the config else it won't know | ||
| # where to look. | ||
| config["root"] = uri.geturl() | ||
| uri = outfile |
There was a problem hiding this comment.
Oops, I didn't realize that config["root"] needed to be updated.
I'm a little nervous about reusing uri to mean first a normalized repository root, and then the config location. Why not have a separate config.dumpToUri(outfile) on this branch? It would be more robust to future code changes, especially ones below this point.
An alternative would be assigning outfile = uri if the argument is None, as this would keep the meaning of both variables unchanged.
There was a problem hiding this comment.
ok.
Yes, if root is not in the config file it no longer knows how to find it's registry/datastore.
tests/test_butler.py
Outdated
| outfile=self.tmpConfigFile) | ||
| print(f"YAML written to {self.tmpConfigFile}") | ||
| c = Config(self.tmpConfigFile) | ||
| print(c) |
There was a problem hiding this comment.
Printing the config isn't really a test. Can you replace this with an assertion (e.g., that the recovered config is equal aside from the root field)?
There was a problem hiding this comment.
It seems that there are two more cases that could be tested: making outfile a directory (if this is not allowed, please say so in the documentation) and making it a URI. I can't think of any obvious failure modes besides invalid/nonwritable location...
There was a problem hiding this comment.
Those print statements are some debugs that should not have been committed.
There was a problem hiding this comment.
Those two options (URI and directory only) do work but now I'm wondering about testing them since one put/get should be a sufficient test whereas at the moment I'm rerunning every butler test (because it's simpler that way). I may have to stop inheriting from the posix datastore test and do a one of test just for makeRepo outfile.
Rather than subclassing all the Butler tests, for the makeRepo test subclass just the main put/get test since we don't need to ensure that all of butler works when tweaking the config location.
timj
force-pushed
the
tickets/DM-23275
branch
from
9990783
to
456c2fc
Compare
|
@kfindeisen I rewrote the tests to add your suggested changes and that made it clear that there was also a bug in posix datastore not working with file URIs. Fixing that meant I no longer needed special case code in registry. |
No description provided.