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

Stop uploading a file if no metadata can be extracted #767

Merged
merged 2 commits into from
Sep 3, 2021
Merged

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Sep 2, 2021

No description provided.

@jwodder jwodder added the patch Increment the patch version when merged label Sep 2, 2021
@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #767 (7481613) into master (49683fe) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #767      +/-   ##
==========================================
- Coverage   85.03%   84.96%   -0.07%     
==========================================
  Files          59       59              
  Lines        6027     6040      +13     
==========================================
+ Hits         5125     5132       +7     
- Misses        902      908       +6     
Flag Coverage Δ
unittests 84.96% <100.00%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
dandi/tests/test_upload.py 100.00% <100.00%> (ø)
dandi/upload.py 84.69% <100.00%> (+1.10%) ⬆️
dandi/support/generatorify.py 0.00% <0.00%> (-16.10%) ⬇️
dandi/dandiapi.py 92.64% <0.00%> (-0.20%) ⬇️
dandi/metadata.py 79.66% <0.00%> (+1.11%) ⬆️
dandi/dandiarchive.py 83.75% <0.00%> (+1.52%) ⬆️

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 49683fe...7481613. Read the comment docs.

@yarikoptic
Copy link
Member

Since quite surprising and "core", please add a unittests (probably a fixture to produce such broken nwb) to ensure that we don't even try to upload then, even upon rerun (thus mimicking scenario in original report which isn't yet troubleshooted into full clarity)

@jwodder
Copy link
Member Author

jwodder commented Sep 3, 2021

@yarikoptic Test added. It only calls upload() once, though, as I really don't think calling it twice is ever going to make a difference.

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Sep 3, 2021
@yarikoptic
Copy link
Member

thank you @jwodder! I thought to toy with it a bit (still hopeful to replicate) but since it fixes a bug, lets proceed with merge and release!

@yarikoptic yarikoptic merged commit b443584 into master Sep 3, 2021
@yarikoptic yarikoptic deleted the bug01 branch September 3, 2021 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants