-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restructure parse_dandi_url() return type #605
Conversation
Codecov Report
@@ Coverage Diff @@
## master #605 +/- ##
==========================================
+ Coverage 83.08% 84.16% +1.08%
==========================================
Files 59 59
Lines 5681 5778 +97
==========================================
+ Hits 4720 4863 +143
+ Misses 961 915 -46
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments. I like how changes look in tests -- much more self-descriptive code ;)
@@ -110,11 +110,11 @@ def ls(paths, schema, metadata, fields=None, format="auto", recursive=False, job | |||
def assets_gen(): | |||
for path in paths: | |||
if is_url(path): | |||
from ..dandiarchive import navigate_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add a test which would cover this code path? I have added raising an exception here and none of the tests flipped anyhow (directly via python or may be through external invocation of dandi ls
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the test just be a smoke test that calls ls
with a URL (pointing to dandiarchive.org or the Docker instance?), or should it also expect something from the output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess with -f json_pp
you could expect nicely formatted record you could loads and if ran on a dandiset with assets -- could check that there is at least one asset (without getting into details)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by -f json_pp
I didn't mean "run in command line"... format="json_pp"
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests added.
from urllib.parse import unquote as urlunquote | ||
|
||
from pydantic import AnyHttpUrl, BaseModel, parse_obj_as, validator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a note: likely because of this module RF and extra imports (but didn't investigate), code of this PR adds ~40 msec to dandi --help
(to the total of 800ms, so about 5%)... at some point we might review import paths etc if we keep growing - since waiting seconds for --help
might be a bit too much, and would be impediment if we add some shell completions support (IIRC I did not find any ready to use for click
such as argcomplete which was odd)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, click has command completion built-in: https://click.palletsprojects.com/en/7.x/bashcomplete/ (though I think some details may be changing in Click 8).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, filed #608 . Feel welcome to self-assign but it is of no urgency so I didn't
Thank you @jwodder -- let's proceed |
Closes #554.
Closes #598.