DM-38492: Support URIs in some additional APIs #813
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #813 +/- ##
==========================================
- Coverage 87.77% 87.74% -0.03%
==========================================
Files 268 268
Lines 34937 34930 -7
Branches 7347 7346 -1
==========================================
- Hits 30667 30651 -16
- Misses 3116 3125 +9
Partials 1154 1154
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
timj
force-pushed
the
tickets/DM-38492
branch
2 times, most recently
from
0134fa6
to
ea8a5bb
Compare
python/lsst/daf/butler/_butler.py
Outdated
| `~lsst.daf.butler.Butler.export`. If this a string (name) or | ||
| `~lsst.resources.ResourcePath` and is not an absolute path, | ||
| does not exist in the current working directory, and ``directory`` | ||
| is not `None`, it is assumed to be in | ||
| ``directory``. Defaults to "export.{format}". |
There was a problem hiding this comment.
Just a general though, not related to the changes - this handling of the directory resembles a search path which always includes two entries - cwd and directory. This may cause confusion if an export file can be found in both places but user really wants it to come from directory, as there is no way to remove cwd from a search path. Would it make sense to reverse search path to look in directory first, or have an option to disable search in cwd?
There was a problem hiding this comment.
I think @TallJimbo designed this API. I'm happy to change it to prefer the given directory over cwd if he agrees.
There was a problem hiding this comment.
I have no problem changing it. Andy's argument makes sense.
There was a problem hiding this comment.
I've changed it to use directory if both cwd and directory have the file. I also issue a warning message in that scenario just in case.
timj
force-pushed
the
tickets/DM-38492
branch
2 times, most recently
from
c3ff71e
to
72a2d0b
Compare
Click doesn't know how to read remote resources so tell it to treat the "export file" parameter as a string and then let ResourcePath deal with it downstream. This also simplifies the test code.
Can not open as a stream because of DM-38589. Read the entire file instead up front.
mypy doesn't like this but it does work.
If someone explicitly specifies a directory and that directory has the export file in it, it seems more likely that that is the one they really wanted.
It is convenient in some cases if URIs can be supported rather than local files.
Checklist
doc/changes