-
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
Recognize video files as non-generic assets #922
Conversation
Codecov Report
@@ Coverage Diff @@
## master #922 +/- ##
==========================================
- Coverage 87.41% 87.31% -0.10%
==========================================
Files 61 62 +1
Lines 7454 7665 +211
==========================================
+ Hits 6516 6693 +177
- Misses 938 972 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@jwodder, @yarikoptic is this good to go? |
@jwodder -- is that all that is needed and PR just needs to be taken out of draft (although having some unittest tuned or added might be good) or there is more to do? |
@yarikoptic You still need to answer my question in the issue: #920. |
dandi/files.py
Outdated
) -> BareAsset: | ||
metadata = get_default_metadata(self.filepath, digest=digest) | ||
metadata.path = self.path | ||
return metadata |
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.
why not to move this implementation of get_metadata
into LocalFileAsset
to provide such default implementation and thus to avoid duplication across subclasses which just need to use the default?
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 there be a test added which would ensure that upload
does recognize video files as "first class citizen"?
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.
get_metadata
: Done.
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.
For testing, what about just checking here that files with one of more of the video extensions are recognized as Dandi files?
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.
ok
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.
Done.
Thank you @jwodder, let's see where the changes would lead us ;) |
Closes #920.