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: remove: Replace interface.save with core.local.save #3586

Merged
merged 2 commits into from Aug 5, 2019

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Aug 5, 2019

rev_save() has been promoted to save(), but there are still a few
places where the obsolete save() is used. One of these spots is
remove.py, where 1c49a1f (MNT: interface.save: Demote and mark
obsolete, 2019-05-14) claimed the old save() is needed because
annotated paths are passed as arguments. But instead of punting until
annotate_paths() use is dropped from remove.py (gh-3368), we can make
the call compatible with the new save() by pulling out the paths from
the annotated paths record.

In addition to dropping the internal use of an obsolete command, this
avoids spurious warnings about removed files not existing.

Closes #3441.


I didn't find any failures with selective testing locally. Let's see how the full Travis build goes.

rev_save() has been promoted to save(), but there are still a few
places where the obsolete save() is used.  One of these spots is
remove.py, where 1c49a1f (MNT: interface.save: Demote and mark
obsolete, 2019-05-14) claimed the old save() is needed because
annotated paths are passed as arguments.  But instead of punting until
annotate_paths() use is dropped from remove.py (dataladgh-3368), we can make
the call compatible with the new save() by pulling out the paths from
the annotated paths record.

In addition to dropping the internal use of an obsolete command, this
avoids spurious warnings about removed files not existing.

Closes datalad#3441.
Given that we now call the Save() from core.local.save, which doesn't
accept annotated paths, and that the goal is to rewrite remove()
without annotate_paths() (dataladgh-3368), this to-do is no longer relevant.

[ci skip]
@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #3586 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3586      +/-   ##
==========================================
- Coverage   76.99%   76.99%   -0.01%     
==========================================
  Files         272      272              
  Lines       35354    35354              
==========================================
- Hits        27222    27221       -1     
- Misses       8132     8133       +1
Impacted Files Coverage Δ
datalad/distribution/remove.py 63.77% <100%> (ø) ⬆️
datalad/interface/annotate_paths.py 77.81% <0%> (-0.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 f833c28...7581891. Read the comment docs.

@kyleam
Copy link
Contributor Author

kyleam commented Aug 5, 2019

OK, the Travis build looks fine, and this is an easy revert if an issue pops up, so merging.

@kyleam kyleam merged commit 7581891 into datalad:master Aug 5, 2019
@kyleam kyleam deleted the remove-use-new-save branch August 5, 2019 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove: warns about removing files which are gone already when rm'ing directory
1 participant