bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE… by soolabettu · Pull Request #891 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits32 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
…rror
Committer: Siddharth Velankar siddharth.velankar@gmail.com
Made changes as per comments from serhiy.storchaka . Havent added an explicit return at the end of the function as it becomes unreachable after my changes, i think.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed a test.
| return exc is not value |
|---|
| # Conform to PEP 8 - "Either all return statements in a function |
| # should return an expression, or none of them should". |
| if exc is not value: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look as an enhancement to me. No need to change this line.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added comment is also incorrect, as exc is not value is already an expression.
| @@ -123,6 +128,7 @@ def __exit__(self, type, value, traceback): |
|---|
| # |
| if sys.exc_info()[1] is not value: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ncoghlan, is it worth to negate the condition and swap raise and return for uniformity with the above code? Shouldn't the function return False instead of None for uniformity?
| @@ -88,7 +88,7 @@ def __exit__(self, type, value, traceback): |
|---|
| try: |
| next(self.gen) |
| except StopIteration: |
| return |
| return None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary as a bare return implicitly returns None.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the other return statements, this should be return False.
| @@ -123,6 +128,7 @@ def __exit__(self, type, value, traceback): |
|---|
| # |
| if sys.exc_info()[1] is not value: |
| raise |
| return None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary as all functions and methods implicitly has a return statement at the end of them.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @serhiy-storchaka's suggestion of making this more consistent with the except RuntimeError case, and making this clause be:
# Use sys.exc_info() for consistency with the contextlib2 backport
if sys.exc_info()[1] is value:
return False
raise
That way all the exception handling clauses are either return <boolean expression> (if suppressing the exception is one of the available options), or else a series of conditional return False statements followed by an unconditional raise.
@brettcannon , please have a look at issue #29692 for a comment regarding conformance to PEP 8 and my subsequent changes to adhere to it, thanks.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main missing piece is a test case to show that the original error no longer happens, but I have a few other minor comments inline.
| return exc is not value |
|---|
| # Conform to PEP 8 - "Either all return statements in a function |
| # should return an expression, or none of them should". |
| if exc is not value: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added comment is also incorrect, as exc is not value is already an expression.
| except RuntimeError as exc: |
|---|
| # Don't re-raise the passed in exception. (issue27122) |
| if exc is value: |
| return False |
| # Likewise, avoid suppressing if a StopIteration exception |
| # was passed to throw() and later wrapped into a RuntimeError |
| # (see PEP 479). |
| if exc.__cause__ is value: |
| if exc.__cause__ is value and type is StopIteration: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the order of the description in the comment, I suggest checking the passed in exception type first:
if type is StopIteration and exc.__cause__ is value:
return False
| @@ -88,7 +88,7 @@ def __exit__(self, type, value, traceback): |
|---|
| try: |
| next(self.gen) |
| except StopIteration: |
| return |
| return None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the other return statements, this should be return False.
| @@ -123,6 +128,7 @@ def __exit__(self, type, value, traceback): |
|---|
| # |
| if sys.exc_info()[1] is not value: |
| raise |
| return None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @serhiy-storchaka's suggestion of making this more consistent with the except RuntimeError case, and making this clause be:
# Use sys.exc_info() for consistency with the contextlib2 backport
if sys.exc_info()[1] is value:
return False
raise
That way all the exception handling clauses are either return <boolean expression> (if suppressing the exception is one of the available options), or else a series of conditional return False statements followed by an unconditional raise.
| @@ -123,6 +128,7 @@ def __exit__(self, type, value, traceback): |
|---|
| # |
| if sys.exc_info()[1] is not value: |
| raise |
| return None |
| else: |
| raise RuntimeError("generator didn't stop after throw()") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given @serhiy-storchaka's comments about readability, I'm wondering if it may make sense to drop some of the else: clauses in this function, such that this final raise ends up clearly being the final unconditionally executed line in the function. At the moment, it's far from clear that every path of execution ends in either return or raise, so it isn't actually possible to "fall off the end" and return None.
Will add test case shortly
#505)
unlucky Unicode characters.
os.fwalk() is sped up by 2 times by using os.scandir().
if pass accept={int, NoneType}.
…Error. (#680)
ValueError always is raised rather than OverflowError for negative counts. Shifting zero with non-negative count always returns zero.
- test_normalization fails if download fails
bpo-29887. The test is still skipped if "-u urlfetch" option is not passed to regrtest (python3 -m test -u urlfetch test_normalization).
- Fix ResourceWarning in test_normalization
bpo-29887: Fix ResourceWarning in test_normalization if tests are interrupted by CTRL+c.
Element.getiterator() and the html parameter of XMLParser() were deprecated only in the documentation (since Python 3.2 and 3.4 correspondintly). Now using them emits a deprecation warning.
- Don’t need check_warnings any more.
…and deque (#887)
when pass indices of wrong type.
Fix the use of recursion in itertools.chain.from_iterable. Using recursion is unnecessary, and can easily cause stack overflows, especially when building in low optimization modes or with Py_DEBUG enabled.
…n urllib.request module. (#918)
s/keys and elements/keys and values/
Changed test code to suppress a compiler warning, while taking care to avoid the code being optimized out by the compiler.
Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined.
Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero.
Fix MemoryError caused by moving around code in PR #886; nbytes was sometimes used unitinitalized (in non-debug builds, when use_calloc was false and elsize was 0).
…ime (#927)
objects when pass out of bound fold argument.
Also uncomment and fix a path test.
…v6. (#879)
the original logic was just comparing the network address but this is wrong because if the network address is equal then we need to compare the ip address for breaking the tie
add more ip_interface comparison tests
Revert "Minor factoring: move redundant resize scaling logic into the resize function."
This reverts commit 4897300.
Implemented review comments and added test case - please review.
Labels
An unexpected behavior, bug, or error