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

Issue 53094: Multi with statement

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 5 months ago by Georg
Modified:
7 years, 5 months ago
Reviewers:
Benjamin
Base URL:
http://svn.python.org/view/*checkout*/python/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 6

Patch Set 2 : Version after review by Benjamin #

Unified diffs Side-by-side diffs Delta from patch set Stats (+195 lines, -68 lines) Patch
M Grammar/Grammar View 1 chunk +2 lines, -2 lines 0 comments Download
M Include/graminit.h View 1 chunk +1 line, -1 line 0 comments Download
M Lib/compiler/transformer.py View 1 chunk +14 lines, -10 lines 0 comments Download
M Lib/test/test_compiler.py View 1 chunk +21 lines, -0 lines 0 comments Download
M Lib/test/test_parser.py View 1 chunk +1 line, -0 lines 0 comments Download
M Lib/test/test_with.py View 1 1 chunk +77 lines, -1 line 0 comments Download
M Modules/parsermodule.c View 1 chunk +18 lines, -15 lines 0 comments Download
M Python/ast.c View 1 2 chunks +41 lines, -20 lines 0 comments Download
M Python/graminit.c View 2 chunks +20 lines, -19 lines 0 comments Download

Messages

Total messages: 2
Benjamin
Looks good except for nits. http://codereview.appspot.com/53094/diff/1/9 File Lib/test/test_compiler.py (right): http://codereview.appspot.com/53094/diff/1/9#newcode186 Line 186: self.assertEquals(dct.get('result'), 1) Why ...
7 years, 5 months ago (2009-05-02 18:22:06 UTC) #1
Georg
7 years, 5 months ago (2009-05-02 18:48:00 UTC) #2
All done, I guess.

http://codereview.appspot.com/53094/diff/1/9
File Lib/test/test_compiler.py (right):

http://codereview.appspot.com/53094/diff/1/9#newcode186
Line 186: self.assertEquals(dct.get('result'), 1)
On 2009/05/02 18:22:06, Benjamin wrote:
> Why using .get() here?

Following the above example.  Probably to not get an error but a test failure if
it isn't there.

http://codereview.appspot.com/53094/diff/1/7
File Lib/test/test_with.py (right):

http://codereview.appspot.com/53094/diff/1/7#newcode657
Line 657: class NestedWith(unittest.TestCase):
On 2009/05/02 18:22:06, Benjamin wrote:
> You should test that the second's __exit__ is called even if the first one
> raises.

Done.

http://codereview.appspot.com/53094/diff/1/3
File Python/ast.c (right):

http://codereview.appspot.com/53094/diff/1/3#newcode3045
Line 3045: assert(TYPE(n) == with_stmt);
On 2009/05/02 18:22:06, Benjamin wrote:
> REQ() is better here.

It was already there, but Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted