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

ls --schema: Calculate digest for local assets #666

Merged
merged 4 commits into from
Jun 14, 2021
Merged

ls --schema: Calculate digest for local assets #666

merged 4 commits into from
Jun 14, 2021

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Jun 11, 2021

Fixes #665.

@jwodder jwodder added the patch Increment the patch version when merged label Jun 11, 2021
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #666 (fa1fc9b) into master (02ee314) will increase coverage by 0.50%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   83.62%   84.12%   +0.50%     
==========================================
  Files          59       59              
  Lines        5379     5606     +227     
==========================================
+ Hits         4498     4716     +218     
- Misses        881      890       +9     
Flag Coverage Δ
unittests 84.12% <95.00%> (+0.50%) ⬆️

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

Impacted Files Coverage Δ
dandi/cli/cmd_ls.py 69.76% <90.00%> (+2.89%) ⬆️
dandi/cli/tests/test_ls.py 100.00% <100.00%> (ø)
dandi/tests/test_metadata.py 100.00% <0.00%> (ø)
dandi/metadata.py 80.38% <0.00%> (+10.38%) ⬆️

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 02ee314...fa1fc9b. Read the comment docs.

@satra
Copy link
Member

satra commented Jun 11, 2021

this works for me. two potential enhancements:

  1. add a test
  2. add a --use_fake_digest option that sticks an appropriate value 32 characters + "-1" for dandi_etag, but doesn't actually compute it.

@jwodder
Copy link
Member Author

jwodder commented Jun 11, 2021

@satra Done.

@@ -60,9 +60,23 @@
help="Convert metadata to new schema version",
metavar="VERSION",
)
@click.option(
Copy link
Member

Choose a reason for hiding this comment

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

Do you think @satra users should be given this opportunity? I would prefer not to... Dear @jwodder , pleae make it a devel option which otherwise would not be visible. Seems to be the first one for ls but we have them for upload etc.

Copy link
Member

Choose a reason for hiding this comment

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

i'm fine with it being a DEVEL option.

Copy link
Member

Choose a reason for hiding this comment

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

but then some kind of output should say "computing checksum" - the pyout output on drogon is not super helpful for me (number of rows displayed, etc.,.). so perhaps a status field could be added that gives such info in a column.

Copy link
Member

Choose a reason for hiding this comment

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

displaying some message is orthogonal to having digest fake or not (that is what the option is about).

why are you interested to get that displayed BTW? I guess we could add some lgr.debug into get_digest for that purpose. We have "mothballed for now" #465 to display digest computing progress during upload. May be after/during Python API refactoring we could come up with some consistent interface across commands to display progress of long going operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made the option devel-only and added an INFO-level log message just before calculating a digest.

Copy link
Member

Choose a reason for hiding this comment

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

it's just that you have no idea as a user what the program is doing when showing ls. just like upload says validating, uploading, complete, it would be nice at least in the pyout display to know that something is going on.

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

ok, let's merge this and release a fresh release of the dandi-cli

@yarikoptic yarikoptic merged commit 2d0476d into master Jun 14, 2021
@yarikoptic yarikoptic deleted the gh-665 branch June 14, 2021 18:34
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.

dandi ls does not work with schema flag
3 participants