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

More correct summary reporting for relaxed (no size) --annex #7050

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 20, 2022

See individual commits for more info. Inspired by #7049

  • first commit - report at least correct number of annexed files
  • 2nd - if relaxed file is present, takes its size from io.stat. But may be we should move that logic into get_content_info or alike?

also thought to tune up bytes2human to not report odd looking float Byte sizes such as 1.0 B instead of 1 B but decided to not bother .

  • add changelog when taking out of draft. Want first to agree on the course of action

@yarikoptic yarikoptic added UX user experience cmd-status labels Sep 20, 2022
@yarikoptic yarikoptic marked this pull request as draft September 20, 2022 20:52
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Sep 20, 2022
@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

❗ No coverage uploaded for pull request base (maint@7db210f). Click here to learn what that means.
Patch coverage: 16.66% of modified lines in pull request are covered.

❗ Current head fb377be differs from pull request most recent head 252eb8f. Consider uploading reports for the commit 252eb8f to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##             maint    #7050   +/-   ##
========================================
  Coverage         ?   75.83%           
========================================
  Files            ?      354           
  Lines            ?    58934           
  Branches         ?     6616           
========================================
  Hits             ?    44693           
  Misses           ?    14227           
  Partials         ?       14           
Impacted Files Coverage Δ
datalad/core/local/status.py 86.72% <16.66%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

In principle seems reasonable to me.
Would prefer to not get the size in the renderer, but it's not instantly clear to me where it should go. Unconditionally in get_content_info seems wrong,too, since this could be quite expensive even when this information isn't needed at all.

@yarikoptic yarikoptic added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jun 9, 2023
@yarikoptic yarikoptic marked this pull request as ready for review June 9, 2023 21:10
@yarikoptic
Copy link
Member Author

there were no objections. Indeed renderer might be a suboptimal spot but we agreed on no better one. Adding changelog as promised.

Inspired by a fix we needed for push:
datalad#7049

I looked into status to realize that we do not even consider those files
for which git-annex would not report bytesize.  This commit makes them
considered in count but would assume 0 bytesize for "recorded" etc.

It would not address the issue that we would still not
consider/interrogate sizes for those files even if their content
available:

	$> datalad status --annex=all
	2 annex'd files (0.0 B/0.0 B present/total size)

	$> ls -lL *
	-r-------- 1 yoh yoh   0 Sep 20 15:52 123.dat
	-r-------- 1 yoh yoh 723 Sep 20 15:57 sample3.csv

where sample3.csv is the relaxed one.
…content

Follow up to previous fix, where we could not provide stats
on size for relaxed files. I guess similar check could be moved
deeper in the code and assign bytesize there, for now did right
at the rendering stage
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jun 9, 2023
@yarikoptic
Copy link
Member Author

appveyor on mac - unrelated

AILED ../datalad/customremotes/tests/test_archives.py::test_basic_scenario - datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmodules=none annex addurl '--file= |;&%b5{}'"'"'"<>Δקم๗あ .dbtc ' 'dl+archive:MD5E-s178--2c93ad202f14e6bfc7c49c281ea99a66.tar.gz#path=+%7C%3B%26%25b5%7B%7D%27%22%3C%3E%CE%94%D7%A7%D9%85%E0%B9%97%E3%81%82+.dbtc+/d/+%7C%3B%26%25b5%7B%7D%27%22%3C%3E%CE%94%D7%A7%D9%85%E0%B9%97%E3%81%82+.datc+' --json --json-error-messages --json-progress -c annex.dotfiles=true' failed with exitcode 139 under /Users/appveyor/DLTMP/datalad_temp_tree_test_basic_scenariorbzzfuki [info keys: stdout_json] [err: 'error: git-annex died of signal 11']

lets' proceed since no objections voiced -- if any comes, submit a PR to remove/redo

@yarikoptic yarikoptic merged commit afcdc16 into datalad:maint Jun 13, 2023
19 of 21 checks passed
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.18.5

@yarikoptic yarikoptic deleted the enh-relaxed branch February 2, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-status semver-patch Increment the patch version when merged UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants