Handle StopIteration in iterable_subprocess
#562
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit modifies `iterable_subprocess` so that it prioritizes `IterableSubprocessError` exceptions over `StopIteration` exceptions, if a `StopIteration` exception is raised in the body of an `iterable_subprocess`-context and if the sub process exits with a non-zero return code. `StopIteration` in conjunction with a non-zero return code indicates an unexpected short read due to an early "error"-exit of the sub process. This commit is a result of this discussion: datalad#546 (comment)
This commit adds tests that verify the intendend behavior of `iterable_subprocess` if a `StopIteration`-exception is raised in the body of an `iterable_subprocess`-context.
This commit ensures that the `LANG` environment variable is set to `C` in a tests that interprets the output of `ls`.
This commit describes the handling of exceptions in the docstring of `iter_subproc`. It highlights the difference between `StopIteration` and other exceptions, in connection with a non-zero subprocess return code.
|
I am wondering whether it would be better to change The instances of this class could then store the return code of the subprocess and it would still be available to us, independent from the exceptions that were raised. Converting this to Draft for now |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #562 +/- ##
==========================================
+ Coverage 92.87% 92.89% +0.01%
==========================================
Files 143 143
Lines 10365 10393 +28
Branches 1141 1147 +6
==========================================
+ Hits 9627 9655 +28
Misses 714 714
Partials 24 24 ☔ View full report in Codecov by Sentry. |
|
Ok, I will create another PR that turns the |
This was referenced Dec 10, 2023
|
With #565 merged, we can close this one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[This has been put into draft-mode in favor of an alternative approach to make return codes available in case of exceptions in PR #565]
This PR implements the result of this discussion in PR #546
It modifies
iterable_subprocessand thereforeiter_subprocto prioritizeIterableSubprocessError-exceptions overStopIteration-exception. The rationale for that is, thatStopIterationand a non-zero exit value of a subprocess indicate that there was a short-read due to an error in the subprocess.Because we require access to the return code of the subprocess, prioritizing
IterableSubprocessError-exceptions overStopIteration-exceptions alleviates the need to catchStopIterationin theiter_subproc-context.