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 docstring about "getting subdatasets" in get #7460

Merged
merged 3 commits into from Aug 1, 2023

Conversation

mslw
Copy link
Contributor

@mslw mslw commented Jul 26, 2023

This docstring-only PR:

  • fixes the name of a property that can be used in subdataset source candidate configuration (remote-<name>remoteurl-<name>, matching the code)
  • updates the default source candidate priorities listed in get docstring (i.e. manpage for get), so that they are the same as in the helper function docstring (_get_flexible_source_candidates_for_submodule().

Regarding the latter: The doctrings for get and its helper function seem to have diverged w.r.t. the default costs for subdataset source candidates. Some changes to cost values happened e.g. in 9138af0. After a cursory look, I am under the impression that the values reported in helper function's docstring match its code (or at least are closer to truth), and as long as they aren't modified elsewhere these would be the valid ones for get.

Fixes #7458

mslw added 2 commits July 26, 2023 15:28
The docstring said the property is remote-<name>, while the code in
_get_flexible_source_candidates_for_submodule() used remoteurl-name.
The doctrings for get and its helper function
_get_flexible_source_candidates_for_submodule() seem to have diverged
w.r.t. the default costs for subdataset source candidates. This commit
brings them back in line by updating get's docstring.

Some changes to cost values happened e.g. in 9138af0

After a cursory look, I am under the impression that the values
reported in helper function's docstring match its code, and as long as
they aren't modified elsewhere these would be the valid ones for get.
@mslw mslw added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jul 26, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jul 26, 2023
@mslw mslw added the semver-documentation Changes only affect the documentation, no impact on version label Jul 26, 2023
@mslw mslw marked this pull request as ready for review July 26, 2023 15:57
@mslw mslw changed the title Doc subdataset candidates Fix docstring about "getting subdatasets" in get Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.60% 🎉

Comparison is base (928498e) 90.73% compared to head (1e42133) 91.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7460      +/-   ##
==========================================
+ Coverage   90.73%   91.34%   +0.60%     
==========================================
  Files         325      325              
  Lines       43399    43399              
  Branches        0     5818    +5818     
==========================================
+ Hits        39380    39642     +262     
+ Misses       4019     3742     -277     
- Partials        0       15      +15     
Files Changed Coverage Δ
datalad/distribution/get.py 96.13% <ø> (+0.77%) ⬆️

... and 38 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@loj loj left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for discovering this!

@mslw
Copy link
Contributor Author

mslw commented Jul 31, 2023

FTR, I see there were errors in the CI runs on CrippledFS 1 and part of Travis runs 2 (though not Appveyor). Given that the PR is docstring-only, I supose they have to be indicative of something unrelated, but I will not have the resources to debug them, and I consider this small PR complete.

Footnotes

  1. FAILED ../tests/test_rerun_merges.py::test_rerun_fastforwardable_mutator

  2. didn't look further, first from top: FAILED ../datalad/customremotes/tests/test_archives.py::test_basic_scenario - datalad.runner.exception.CommandError

@yarikoptic
Copy link
Member

Looks LGTM to me, let's proceed!

@yarikoptic yarikoptic merged commit 9fe3f9e into datalad:maint Aug 1, 2023
21 of 24 checks passed
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.19.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-documentation Changes only affect the documentation, no impact on version
Projects
None yet
4 participants