Skip to content
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

RF: attempt nwb metadata extraction only on .nwb, if fails -- warning #733

Merged
merged 2 commits into from
Jul 23, 2021

Conversation

yarikoptic
Copy link
Member

I think time to assume that all NWB files would have .nwb extension I think.

This PR is maintaining prior behavior of re-raising the exception if allow_any_path is
False. Otherwise -- and regardless if allow_any_path is True or not (thus
would not re-raise for non .nwb files even if not allow_any_path) - provide default
metadata record.

For upload it should be fine AFAIK even with the change of behavior since we first
use allow_any_path to select .nwb files.

Closes #730

Maintaining prior behavior of re-raising the exception if allow_any_path is
False.  Otherwise -- and regardless if allow_any_path is True or not (thus
would not re-raise for non .nwb files even if not allow_any_path) - provide default
metadata record.

For upload it should be fine AFAIK even with the change  of behavior since we first
use allow_any_path to select .nwb files.

Closes #730
@yarikoptic yarikoptic added the minor Increment the minor version when merged label Jul 22, 2021
@yarikoptic yarikoptic requested review from jwodder and satra July 22, 2021 22:45
@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #733 (5d3ef59) into master (184cc30) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
- Coverage   84.12%   84.09%   -0.03%     
==========================================
  Files          59       59              
  Lines        5699     5702       +3     
==========================================
+ Hits         4794     4795       +1     
- Misses        905      907       +2     
Flag Coverage Δ
unittests 84.09% <60.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandi/metadata.py 78.55% <60.00%> (-0.67%) ⬇️
dandi/dandiapi.py 90.93% <0.00%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 184cc30...5d3ef59. Read the comment docs.

)
else:
if not allow_any_path:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be indented one level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this. otherwise LGTM

Happens to the best of us
@yarikoptic yarikoptic merged commit 457c1e5 into master Jul 23, 2021
@yarikoptic yarikoptic deleted the enh-metadata-nwb branch July 23, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adjust nwb metadata extraction only for nwb files
3 participants