Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(10)

#22906: PEP 479: Change StopIteration handling inside generators

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 2 months ago by rosuav
Modified:
4 years, 8 months ago
Reviewers:
berker.peksag, ncoghlan, storchaka, yselivanov
CC:
gvanrossum, rhettinger, Nick Coghlan, sasha, scoder, haypo, r.david.murray, stoneleaf, Yury.Selivanov, devnull_psf.upfronthosting.co.za, schlamar, Rosuav, berkerpeksag, storchaka, Yury Selivanov, neil.g, john.black.3k_gmail.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Total comments: 20

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/howto/functional.rst View 1 2 3 4 5 6 1 chunk +4 lines, -4 lines 0 comments Download
Doc/library/exceptions.rst View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
Doc/library/__future__.rst View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
Doc/reference/expressions.rst View 1 2 3 4 5 6 1 chunk +6 lines, -6 lines 0 comments Download
Include/code.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
Include/compile.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
Include/pythonrun.h View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
Lib/contextlib.py View 1 2 3 4 5 6 1 chunk +9 lines, -2 lines 0 comments Download
Lib/difflib.py View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
Lib/__future__.py View 1 2 3 4 5 6 3 chunks +6 lines, -0 lines 0 comments Download
Lib/test/test_contextlib.py View 1 2 3 4 5 6 1 chunk +34 lines, -0 lines 0 comments Download
Lib/test/test_pep479.py View 1 2 3 4 5 6 1 chunk +45 lines, -0 lines 0 comments Download
Objects/genobject.c View 1 2 3 4 5 6 1 chunk +24 lines, -0 lines 0 comments Download
Python/future.c View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 4
berkerpeksag
Could you also add some tests to Lib/test/test_generators.py?
5 years, 2 months ago #1
Nick Coghlan
This mostly looks good to me, but the test updates mentioned in the tracker comment ...
5 years ago #2
storchaka_gmail.com
https://bugs.python.org/review/22906/diff/13560/Lib/contextlib.py File Lib/contextlib.py (right): https://bugs.python.org/review/22906/diff/13560/Lib/contextlib.py#newcode85 Lib/contextlib.py:85: # Likewise, suppress if that StopException got wrapped up. ...
4 years, 10 months ago #3
Yury Selivanov
4 years, 8 months ago #4
Thanks for the review! Most of the feedback is in the new patch.

http://bugs.python.org/review/22906/diff/13560/Doc/library/exceptions.rst
File Doc/library/exceptions.rst (right):

http://bugs.python.org/review/22906/diff/13560/Doc/library/exceptions.rst#new...
Doc/library/exceptions.rst:313: If a generator function created in the presence
of a ``from __future__
On 2015/01/09 10:07:19, Nick Coghlan wrote:
> I suggest using the phrasing "If the body of a generator function ..."

It also sounds a bit confusing :(

Maybe we should use "defined" instead of "created"?

http://bugs.python.org/review/22906/diff/13560/Lib/difflib.py
File Lib/difflib.py (left):

http://bugs.python.org/review/22906/diff/13560/Lib/difflib.py#oldcode1550
Lib/difflib.py:1550: while True:
Why is this necessary?

http://bugs.python.org/review/22906/diff/13560/Lib/difflib.py
File Lib/difflib.py (right):

http://bugs.python.org/review/22906/diff/13560/Lib/difflib.py#newcode1519
Lib/difflib.py:1519: try:
Indentation must be 4 spaces, not 2.

Shouldn't you actually put try..except only over the "next()" call?

http://bugs.python.org/review/22906/diff/13560/Lib/difflib.py#newcode1559
Lib/difflib.py:1559: try:
Same thing -- 2 spaces instead of 4, and too much code in the try..except.

http://bugs.python.org/review/22906/diff/13560/Objects/genobject.c
File Objects/genobject.c (right):

http://bugs.python.org/review/22906/diff/13560/Objects/genobject.c#newcode135
Objects/genobject.c:135: /* Check for __future__ generator_stop and
conditionally turn a leaking
On 2015/03/06 00:58:05, storchaka wrote:
> Line is too long.

Fixed.

http://bugs.python.org/review/22906/diff/13560/Objects/genobject.c#newcode137
Objects/genobject.c:137: && (((PyCodeObject *)gen->gi_code)->co_flags &
CO_FUTURE_GENERATOR_STOP)) {
+1

http://bugs.python.org/review/22906/diff/13560/Objects/genobject.c#newcode142
Objects/genobject.c:142: PyException_SetTraceback(val, tb);
On 2015/03/06 00:58:05, storchaka wrote:
> Why this is needed? Isn't traceback set in PyErr_NormalizeException(). May be
> _PyErr_ChainException() needs this too?


Looks like it does not:
    XXX: should PyErr_NormalizeException() also call
            PyException_SetTraceback() with the resulting value and tb?
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+