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

RF: update() no longer uses AnnotatePaths #3700

Merged
merged 5 commits into from Sep 24, 2019
Merged

RF: update() no longer uses AnnotatePaths #3700

merged 5 commits into from Sep 24, 2019

Conversation

@mih
Copy link
Member

@mih mih commented Sep 22, 2019

This also introduce slight changes in the API semantics, by removing
needless flexibility (ie. specifying a to-be-updated dataset either
via -d or as a path). Multi-seed recursive operation is also no longer
supported, in line with previous changes.

The PATH arg is now only used to constrain the updating of subdatasets
in recursive mode.

ping gh-3368

This also introduce slight changes in the API semantics, by removing
needless flexibility (ie. specifying a to-be-updated dataset either
via -d or as a path). Multi-seed recursive operation is also no longer
supported, in line with previous changes.

The PATH arg is now only used to constrain the updating of subdatasets
in recursive mode.

ping dataladgh-3368
@codecov
Copy link

@codecov codecov bot commented Sep 22, 2019

Codecov Report

Merging #3700 into master will increase coverage by 0.11%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3700      +/-   ##
=========================================
+ Coverage   82.78%   82.9%   +0.11%     
=========================================
  Files         273     273              
  Lines       35808   35790      -18     
=========================================
+ Hits        29644   29671      +27     
+ Misses       6164    6119      -45
Impacted Files Coverage Δ
datalad/distribution/tests/test_update.py 100% <ø> (ø) ⬆️
datalad/distribution/update.py 71.42% <90%> (+3.2%) ⬆️
datalad/downloaders/tests/test_http.py 61.23% <0%> (+1.23%) ⬆️
datalad/interface/run_procedure.py 88.81% <0%> (+19.87%) ⬆️

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 c7aa628...c5d571f. Read the comment docs.

datalad/distribution/update.py Outdated Show resolved Hide resolved
@@ -48,7 +58,8 @@ class Update(Interface):
path=Parameter(
args=("path",),
metavar="PATH",
doc="path to a dataset that shall be updated",
doc="""contrain to-be-updated subdatasets to the given path for recursive
operation.""",
Copy link
Contributor

@kyleam kyleam Sep 23, 2019

Choose a reason for hiding this comment

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

AFAICS path isn't considered in the downstream code (and quickly testing, it doesn't seem to limit the recursive operations to a given path). I'm guessing it's supposed to be passed to the subdatasets call.

And if paths is just to be passed directly to subdatasets, perhaps we should support multiple restrictions by changing this to nargs='*'.

Copy link
Member Author

@mih mih Sep 24, 2019

Choose a reason for hiding this comment

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

Erm, embarrasing... I am now passing path on to subdatasets. However, the nargs setting was already in place and wasn't changed.

Copy link
Contributor

@kyleam kyleam Sep 24, 2019

Choose a reason for hiding this comment

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

However, the nargs setting was already in place and wasn't changed.

Right. Sorry for the noise.

# fail when asked to update a something non-existent
assert_status(
'impossible',
source.update("nothere", on_failure='ignore'))
Copy link
Contributor

@kyleam kyleam Sep 23, 2019

Choose a reason for hiding this comment

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

My reading of this: path is now only intended to restrict which subdatasets we recurse into; the reference dataset is always updated. Instead of the old behavior of failing, the path argument is (or should be, see below) passed to subdatasets and if the path doesn't lead to subdatasets (because it isn't a dataset itself, isn't a directory with datasets, or doesn't exist) then no subdatasets will be updated. It seems a bit sad that there's no feedback about the paths being non-functional, but it would be in line with subdatasets current behavior. OK.

Copy link
Member Author

@mih mih Sep 24, 2019

Choose a reason for hiding this comment

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

I added a warning in case no subdataset was processed when a path constraint was specified.

Copy link
Member Author

@mih mih Sep 24, 2019

Choose a reason for hiding this comment

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

Some additional tests on path constraints are no added.

datalad/distribution/update.py Outdated Show resolved Hide resolved
kyleam
kyleam approved these changes Sep 24, 2019
Copy link
Contributor

@kyleam kyleam left a comment

Thanks for the updates.

@mih
Copy link
Member Author

@mih mih commented Sep 24, 2019

Great, thanks!

@mih mih merged commit ada0c9e into datalad:master Sep 24, 2019
3 checks passed
@mih mih deleted the rf-update branch Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants