bpo-35105: Document that CPython accepts "invalid" identifiers#11263
bpo-35105: Document that CPython accepts "invalid" identifiers#11263Windsooon wants to merge 2 commits into
Conversation
Sorry, something went wrong.
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
That you for proposing something specific to chew on. I think spelling, grammar, and content all need tweaking.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
Do we really need to mention this? |
Sorry, something went wrong.
|
That discussion is on the issue. |
Sorry, something went wrong.
rhettinger
left a comment
rhettinger
left a comment
There was a problem hiding this comment.
I don't think we can mark this as an implementation detail for setattr(). The details are downstream and determined by the target object, not by setattr itself.
Suggested wording:
Note, setattr() attempts to update the object with the given attr/value pair.
Whether this succeeds and what its affect is is determined by the target object.
If an object's class defines `__slots__`, the attribute may not be writeable.
If an object's class defines property with a setter method, the *setattr()*
will trigger the setter method which may or may not actually write the attribute.
For objects that have a regular dictionary (which is the typical case), the
*setattr()* call can make any string keyed update allowed by the dictionary
including keys that aren't valid identifiers -- for example setattr(a, '1', 'one')
will be the equivalent of vars()['1'] ='one'.
This issue has little to do with setattr() and is more related to the fact that instance dictionaries can hold any valid key. In a way, it is no different than a user writing a.__dict__['1'] = 'one'. That has always been allowed and the __dict__ attribute is documented as writeable, so a user is also allowed to write `a.dict = {'1': 'one'}.
In short, we can talk about this in the setattr() docs but it isn't really a setattr() issue. Also, the behavior is effectively guaranteed by the other things users are allowed to do, so there is no merit in marking this as an implementation detail. Non-identifier keys can make it into an instance dictionary via multiple paths that are guaranteed to work.
Sorry, something went wrong.
|
I agree with Raymond's revision. Note that it would replace the entire proposed addition, including the impl-detail directive. By adding more 'meat', it makes an addition more worthwhile. |
Sorry, something went wrong.
csabella
left a comment
csabella
left a comment
There was a problem hiding this comment.
The whole note needs to be indented for proper alignment.
Sorry, something went wrong.
|
Hi @Windsooon thanks for this PR! Would you have some time to integrate Cheryl comments? |
Sorry, something went wrong.
JulienPalard
left a comment
JulienPalard
left a comment
There was a problem hiding this comment.
LGTM with nits (all can be accepted right from github).
Sorry, something went wrong.
|
@Windsooon, please address the review comments from Julien. Thanks! |
Sorry, something went wrong.
|
Changes were requested on this PR by a core dev over two years ago, but have not been applied. There is also now a merge conflict. I am therefore closing this PR. @Windsooon, if you're still interested in working on this, feel free to open a new PR. Alternatively, ping me, and I'll happily reopen the PR. Thanks! 🙂 |
Sorry, something went wrong.
|
@AlexWaygood I'm so sorry for missing the notification from CPython, I will catch up in a few weeks. |
Sorry, something went wrong.
Reviewers
11 participants
https://bugs.python.org/issue35105