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

BF: save: Honor on_failure #3470

Merged
merged 1 commit into from Jun 25, 2019
Merged

BF: save: Honor on_failure #3470

merged 1 commit into from Jun 25, 2019

Conversation

@kyleam
Copy link
Contributor

@kyleam kyleam commented Jun 3, 2019

save() calls status() with the default on_failure value of "continue",
which means that---regardless of the on_failure value specified for
save()---status() always raises an IncompleteResultsError exception
when a failure occurs and then save() aborts. Make save() call
status() with on_failure="ignore" so that the save caller gets to
decide what to do in the case of a failure.

Note that this results in some double reporting:

[ERROR  ] path not underneath this dataset [status(...)]
save(error): ../b [path not underneath this dataset]

But if save() were to just swallow the result so that only the log
message is shown, an on_failure value of "continue" or "stop" would
not raise an IncompleteResultsError exception or exit on the command
line with a non-zero status.

@kyleam
Copy link
Contributor Author

@kyleam kyleam commented Jun 3, 2019

Ran into this when going down a rabbit hole for gh-3468.

@codecov
Copy link

@codecov codecov bot commented Jun 3, 2019

Codecov Report

Merging #3470 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3470      +/-   ##
==========================================
+ Coverage   91.26%   91.27%   +0.01%     
==========================================
  Files         272      272              
  Lines       34903    34910       +7     
==========================================
+ Hits        31853    31865      +12     
+ Misses       3050     3045       -5
Impacted Files Coverage Δ
datalad/core/local/tests/test_save.py 100% <100%> (ø) ⬆️
datalad/core/local/save.py 98.66% <100%> (+0.05%) ⬆️
datalad/downloaders/tests/test_http.py 92.32% <0%> (+1.23%) ⬆️

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 3fd8b2a...14df231. Read the comment docs.

datalad/core/local/save.py Outdated Show resolved Hide resolved
save() calls status() with the default on_failure value of "continue",
which means that---regardless of the on_failure value specified for
save()---status() always raises an IncompleteResultsError exception
when a failure occurs and then save() aborts.  Make save() call
status() with on_failure="ignore" so that the *save* caller gets to
decide what to do in the case of a failure.

Note that this results in some double reporting:

    [ERROR  ] path not underneath this dataset [status(...)]
    status(error): ../a [path not underneath this dataset]

But if save() were to just swallow the result so that only the log
message is shown, an on_failure value of "continue" or "stop" would
not raise an IncompleteResultsError exception or exit on the command
line with a non-zero status.
Copy link
Member

@mih mih left a comment

Sorry, late to the party. Below my comment, but treat with caution -- I did not actually run/try anything.

datalad/core/local/save.py Show resolved Hide resolved
@mih mih merged commit 7e48624 into datalad:master Jun 25, 2019
6 checks passed
@kyleam kyleam deleted the save-error branch Jun 25, 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

3 participants