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

Fix AnnexRepo.whereis key=True mode operation, and add batch mode support #6379

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Jan 26, 2022

First commit is truly a BF and destined to maint. The 2nd addresses the limitation (we were just raising a ValueError, not even NotImplementedError) if asked to perform in batch mode on keys. Support for that functionality was added only recently to git-annex, and since change is small, I have decided to bundle it within the same PR against maint. But I would be ok to split it into master. Motivation -- a need for datalad-fuse development.

attn @jwodder (I will provide more context in datalad-fuse later)

Changelog

🐛 Bug Fixes

  • Fixes AnnexRepo.whereis operation in key=True mode whenever multiple keys are provided.

💫 Enhancements and new features

  • Adds support for batch mode operation to AnnexRepo.whereis in key=True mode (requires git-annex 8.20210903 or later)

…ually"

apparently no test was excercising it and no code was using where is
on multiple keys at once.  Detected this defect while working out the
"batch" mode for whereis

also adjusted the test for "conflicting" invocation of --all with
--key option.  Because we loop explicitly now, we would not even
invoke git-annex and thus not trigger the error if test is invoked
without any file.  So I just decided to adjust the test since IMHO it
would better reflect needed testing of us still bubbling up the
exception.

FWIW, filed an issue on --all not erroring out with --batch-keys:

https://git-annex.branchable.com/bugs/should_error_on_whereis_--batch-keys_--all/?updated
…nex)

Recently (in 8.20210903) git-annex introduced --batch-keys option to a
number of commands which otherwise had --batch mode, but not for
whenever specification was on keys instead of paths. I thought to make
use of whereis(..., key=True, batch=True) mode of operation to only
then discover that we do not have that supported.
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jan 26, 2022
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #6379 (a1c4b2a) into maint (e945d2f) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #6379      +/-   ##
==========================================
- Coverage   90.17%   90.17%   -0.01%     
==========================================
  Files         312      312              
  Lines       42270    42267       -3     
==========================================
- Hits        38116    38113       -3     
  Misses       4154     4154              
Impacted Files Coverage Δ
datalad/support/annexrepo.py 90.92% <100.00%> (+0.04%) ⬆️
datalad/support/tests/test_annexrepo.py 97.63% <100.00%> (+0.01%) ⬆️
datalad/downloaders/tests/test_http.py 89.17% <0.00%> (-1.89%) ⬇️
datalad/cmdline/tests/test_main.py 87.89% <0.00%> (+3.32%) ⬆️

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 e945d2f...a1c4b2a. Read the comment docs.

@yarikoptic
Copy link
Member Author

eh, codecov report of -3 lines coverage effect makes little sense to me:

@mih mih added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jan 28, 2022
@yarikoptic yarikoptic changed the title BF+ENH: AnnexRepo.whereis, key=True mode - correct operation + batch mode support Fix AnnexRepo.whereis key=True mode operation, and add batch mode support Feb 8, 2022
@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Feb 8, 2022
@yarikoptic
Copy link
Member Author

@datalad/developers this PR is ready for review and potentially a merge. Your feedback is always welcome!

@mih
Copy link
Member

mih commented Feb 8, 2022

Can we get a changelog @yarikoptic ?

@yarikoptic
Copy link
Member Author

Can we get a changelog @yarikoptic ?

this PR is against maint , so will not be participating in master release directly. Do I still need to add a changelog entry?

Moreover we are at 0.15.4-22-g967ff5da7 in maint so may be worth even kicking out a release with this PR -- there were some bugfixes, no big need/desire to hold them away from seeing a wild world.

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Feb 8, 2022
@yarikoptic
Copy link
Member Author

Attn @mih: FWIW added Changelog entries to the main description.

@mih
Copy link
Member

mih commented Feb 9, 2022

Thx!

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

LGTM!

@mih mih merged commit f539910 into datalad:maint Feb 9, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CHANGELOG-missing When a PR's description does not contain a changelog item, yet. merge-if-ok OP considers this work done, and requests review/merge release Create a release when this pr is merged semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants