Skip to content

bpo-17422:language reference should specify restrictions on class namespace #18559

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Feb 22, 2020
Merged

bpo-17422:language reference should specify restrictions on class namespace #18559

merged 12 commits into from
Feb 22, 2020

Conversation

ananthan-123
Copy link
Contributor

@ananthan-123 ananthan-123 commented Feb 19, 2020 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Feb 19, 2020
@mdickinson mdickinson removed their request for review February 19, 2020 18:27
brandtbucher
brandtbucher requested changes Feb 21, 2020
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Thanks @ananthan-123. No need to request reviews from 3 triagers and 4 core developers though, that's bordering on spam (especially if they have nothing to do with the issue/patch). One request should be fine, maybe a second if the first person doesn't respond after a week or two.

A few things I spotted here (I'm assuming that the wording in Eric's original patch from 7 years ago is correct):

brandtbucher
brandtbucher approved these changes Feb 21, 2020
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good now. @ericsnowcurrently, care to review the content for this? It's your issue.

brandtbucher
brandtbucher requested changes Feb 21, 2020
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Sorry, one more thing...

@ananthan-123 @brandtbucher
Update Misc/NEWS.d/next/Documentation/2020-02-19-11-13-47.bpo-17422.g…
1c648cd 
…7_9zz.rst

Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
gvanrossum
gvanrossum approved these changes Feb 22, 2020
View reviewed changes
Misc/NEWS.d/next/Documentation/2020-02-19-11-13-47.bpo-17422.g7_9zz.rst Outdated
@@ -0,0 +1 @@
Language reference should specify restrictions on class namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Please attribute the patch to its author here.

@gvanrossum gvanrossum merged commit fbe2e0b into python:master Feb 22, 2020
@bedevere-bot
Copy link

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Thanks @ananthan-123 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @ananthan-123 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-18610 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 22, 2020
@ananthan-123 @miss-islington
bpo-17422: Language reference should specify restrictions on class na…
dd99fb4 
…mespace (pythonGH-18559)

The language reference now specifies restrictions on class namespaces.  Adapted from a patch by Ethan Furman.
(cherry picked from commit fbe2e0b)

Co-authored-by: ananthan-123 <ananthakrishnan15.2001@gmail.com>
@bedevere-bot
Copy link

GH-18611 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request Feb 22, 2020
@miss-islington @ananthan-123
bpo-17422: Language reference should specify restrictions on class na…
91ba446 
…mespace (GH-18559)

The language reference now specifies restrictions on class namespaces.  Adapted from a patch by Ethan Furman.
(cherry picked from commit fbe2e0b)

Co-authored-by: ananthan-123 <ananthakrishnan15.2001@gmail.com>
miss-islington added a commit that referenced this pull request Feb 22, 2020
@miss-islington @ananthan-123
bpo-17422: Language reference should specify restrictions on class na…
36a120d 
…mespace (GH-18559)

The language reference now specifies restrictions on class namespaces.  Adapted from a patch by Ethan Furman.
(cherry picked from commit fbe2e0b)

Co-authored-by: ananthan-123 <ananthakrishnan15.2001@gmail.com>
ericsnowcurrently
ericsnowcurrently reviewed Feb 28, 2020
View reviewed changes
Doc/reference/datamodel.rst

If the metaclass has no ``__prepare__`` attribute, then the class namespace
is initialised as an empty ordered mapping.
Copy link
Member

Choose a reason for hiding this comment

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

Per PEP 520, the default class namespace must be an ordered mapping and the original text here was purposefully phrased. While we've talked about requiring "dict" to be ordered (in the language reference), that hasn't happened. So changing the language spec text here to "dict" is inappropriate without a PEP. Note that elsewhere in the language reference the "ordered mapping" phrasing still exists.

Copy link
Member

Choose a reason for hiding this comment

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

Um, dict is required to be ordered. Since 3.7. See the 3.7. what's new doc "the insertion-order preservation nature of dict objects has been declared to be an official part of the Python language spec."

If after that has sunk in you still think this PR is wrong, please submit a new PR to fix it! (Sorry for landing prematurely.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Per my comment on the bpo I think @ericsnowcurrently is right but for the wrong reasons. I have opened a PR correcting the wording. #18682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

@cdonovick cdonovick cdonovick left review comments

@brandtbucher brandtbucher brandtbucher approved these changes

Assignees
No one assigned
Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ananthan-123 @bedevere-bot @miss-islington @ericsnowcurrently @gvanrossum @cdonovick @brandtbucher @the-knights-who-say-ni