◐ Shell
reader mode source ↗
Skip to content

bpo-32012: Disallow trailing comma after genexpr without parenthesis.#4382

Merged
serhiy-storchaka merged 8 commits into
python:masterfrom
serhiy-storchaka:gen-expr-argument-and-trailing-comma
Nov 15, 2017
Merged

bpo-32012: Disallow trailing comma after genexpr without parenthesis.#4382
serhiy-storchaka merged 8 commits into
python:masterfrom
serhiy-storchaka:gen-expr-argument-and-trailing-comma

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Nov 12, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka force-pushed the gen-expr-argument-and-trailing-comma branch from b0bfcb7 to db4aafc Compare November 12, 2017 22:44
ncoghlan
ncoghlan approved these changes Nov 13, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

The test and compiler updates look good to me, just some minor grammar suggestions for the NEWS entry.

@serhiy-storchaka

serhiy-storchaka commented Nov 13, 2017

Copy link
Copy Markdown
Member Author

Thank you @ncoghlan for your fixes.

ncoghlan
ncoghlan requested changes Nov 14, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I think this version goes too far - decorator factories that accept generator expressions don't break any syntactic design principles, and we don't check for any other "definitely not a valid base class" expressions in base class lists.

(The latter change might be an interesting one to make, but it should be a separate enhancement request that checks for all the constructs with a guaranteed non-class return type, and explicitly considers the question of metaclasses that might be doing something unusual with the base class list)

@bedevere-bot

bedevere-bot commented Nov 14, 2017

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

benjaminp
benjaminp reviewed Nov 14, 2017
View reviewed changes

@benjaminp benjaminp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

This seems to disallow a lot more than trailing commas in call genexps.

@serhiy-storchaka

serhiy-storchaka commented Nov 14, 2017

Copy link
Copy Markdown
Member Author

This seems to disallow a lot more than trailing commas in call genexps.

Only makes the implementation more conforming to the specification.

@serhiy-storchaka

serhiy-storchaka commented Nov 14, 2017

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

@bedevere-bot

bedevere-bot commented Nov 14, 2017

Copy link
Copy Markdown

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.

ncoghlan
ncoghlan approved these changes Nov 15, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Thanks! We can follow up on the others on the issue tracker, and potentially take them to python-dev if Guido thinks that's appropriate.

@serhiy-storchaka serhiy-storchaka merged commit 9165f77 into python:master Nov 15, 2017
@serhiy-storchaka

serhiy-storchaka commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

Thank you for your review @ncoghlan and @benjaminp!

@serhiy-storchaka

serhiy-storchaka commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

Created #4400 for fixing class definitions.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Nov 15, 2017
Merged
tacaswell added a commit to tacaswell/toolz that referenced this pull request Nov 16, 2017
@tacaswell
FIX: compatibility with python3.7 …
77ac86f 
This expression is broken by:

bpo-32012: Disallow trailing comma after genexpr without
parenthesis.

python/cpython#4382
python/cpython@9165f77
@tacaswell tacaswell mentioned this pull request Nov 16, 2017
Merged
tacaswell added a commit to tacaswell/toolz that referenced this pull request Nov 16, 2017
@tacaswell
FIX: compatibility with python3.7 …
5eb9fcb 
This expression is broken by:

bpo-32012: Disallow trailing comma after genexpr without
parenthesis.

python/cpython#4382
python/cpython@9165f77
@serhiy-storchaka serhiy-storchaka deleted the gen-expr-argument-and-trailing-comma branch September 22, 2018 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjaminp benjaminp benjaminp left review comments

@ncoghlan ncoghlan ncoghlan approved these changes

Assignees

No one assigned

Labels

A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@serhiy-storchaka @bedevere-bot @benjaminp @ncoghlan @the-knights-who-say-ni