-
Notifications
You must be signed in to change notification settings - Fork 110
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: Perform result rendering prior error handling #3863
Conversation
ea69932
to
15deff6
Compare
Test failures are unrelated. |
Could you provide a snippet or test to demonstrate a scenario this describes? Is "act on" restricted to callers that are inspecting the rendered output?
Before this change, this snippet
produces
After it produces
Makes sense. |
@kyleam thanks for answering this question. This illustrates the core of the matter. I actually stumbled upon this issue, because I have result logging disabled locally, and ran a |
Oh, I actually wasn't intending to answer my own question :) With that snippet, I was just trying to work through the other part of the commit message, the quoted bit right above. The main thing I wasn't sure about and was trying to clarify with the question is whether "not being able to act on" is about a caller not getting the rendered result on stdout, or whether the ordering here somehow affects Python callers working with the returned results (I don't think that's the case).
Got it. The change in this PR looks right, but I think for
Anyway, assuming nothing that I wrote above reveals a misunderstanding on my part, this code movement looks correct to me. |
Arrgh, I missed the filter... thx! Will have a closer look. |
Changes here are broken. They will actually cause dataset to not give a non-zero exit code on create error. Not yet sure how exactly. |
It comes down to datalad/datalad/interface/utils.py Lines 553 to 560 in 15deff6
And that means it never appends to datalad/datalad/interface/utils.py Line 592 in 15deff6
|
Otherwise a user might not become aware of the specific nature of error results (depending on 'on-failure' setting), because we will not get them to a result renderer.
15deff6
to
9eac224
Compare
Codecov Report
@@ Coverage Diff @@
## master #3863 +/- ##
==========================================
+ Coverage 71.01% 80.15% +9.14%
==========================================
Files 272 273 +1
Lines 36065 36012 -53
==========================================
+ Hits 25611 28867 +3256
+ Misses 10454 7145 -3309
Continue to review full report at Codecov.
|
I have force-pushed on update that maintains the sequence of error handling and result filtering in this code move. This should address the remaining issue. Moreover, I have edited the commit message to no longer talk about a vague "act on":
|
Windows test failure looks familiar and unrelated. appveyor died on coverage submission, but passes otherwise. |
Otherwise we might not be able to act on error results (depending on 'on-failure' setting), and also will not get them to a result renderer.
This is a surprisingly old problem.