Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-96397: Document that keywords in calls need not be identifiers#96393

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.

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

Already on GitHub?Sign in to your account

Merged
gvanrossum merged 4 commits intopython:mainfromjeff5:doc-accept-nonid-kwargs
Sep 22, 2022

Conversation

jeff5
Copy link
Contributor

@jeff5jeff5 commentedAug 29, 2022
edited by merwok
Loading

This is to clarify that the keys of keyword arguments (necessarily given as
a mapping) are acceptable in a call even if they would not be valid
as the identifiers of formal parameters. It responds to the discussion at:

https://discuss.python.org/t/supporting-or-not-invalid-identifiers-in-kwargs/17147/31

Respectfully note the continuing desire of some participants there there to debate this further.

Also, I may not have done full justice to@ChrisBarker-NOAA's contribution. In particular, I think it is worth documenting, as a further changeset here or on another PR, the apparent acceptability of non-identifier names in the__dict__ of objects, supported by(get|set)attr, which he pointed out.

We clarify that the keys of keyword arguments (necessarily given asa mapping) are acceptable in a call even if they would not be validas the identifiers of formal parameters.
@merwok
Copy link
Member

Let’s create an issue so that it can be referenced properly!

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

WIth two small wording nits I am in favor of this PR. Given the controversy around it I will request a SC member as a second reviewer.

@gvanrossumgvanrossum requested a review fromYhg1sAugust 29, 2022 18:19
Copy link
Member

@pfmoorepfmoore left a comment

Choose a reason for hiding this comment

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

Guido's suggestions are the only comments I would have made.

@gvanrossum
Copy link
Member

Let’s create an issue so that it can be referenced properly!

Agreed.@merwok could you create the issue?

merwok reacted with thumbs up emoji

@nascheme
Copy link
Member

I agree this is behaviour of CPython that we don't intend to change. I.e. it is not a bug. However, is it a feature of the Pythonlanguage that these non-identifier keywords are allowed or is it an implementation detail of CPython? The reality seems to be that other Python implementations like pypy and Graal have to do the same thing, in order to be compatible. It seems the trend is to avoid having undefined behaviour in languages. So, maybe it should be defined at the language level.

gpshead and Fidget-Spinner reacted with thumbs up emoji

@merwokmerwok changed the titleDocument that keywords in calls need not be identifiersgh-96397: Document that keywords in calls need not be identifiersAug 29, 2022
@merwok
Copy link
Member

Issue created:#96397

@merwokmerwok linked an issueAug 29, 2022 that may beclosed by this pull request
@ChrisBarker-NOAA
Copy link
Contributor

ChrisBarker-NOAA commentedAug 29, 2022
edited
Loading

@nascheme: There was a bunch of discussion about that in discourse, I don't think any consensus was reached.[*]

Once an issue has been created, I guess that's where we could discuss it.

OOPS -- I see it was created now -- I'll go there.

I'd also like to document the same thing with__dict__, setattr(), getattr() (anywhere else?) -- but I just looked through the docs, and I can't see where it would fit -- it's seems it belongs in the Language Reference, but I can't find where to put it -- maybe that can be discussed in the issue as well.

[*} My $0.02: using non-valid identifiers as attribute names via__dict__, **kwargs, getattr(), setattr() is being done in live production code right now, at least in cPython (https://pypi.org/project/netCDF4/ and I'm sure others). I have no idea what other implementations are doing, but I suspect it's the same, at least with PyPy. Given that, regardless of original intent -- I think it's better to define this as a language feature rather than an implementation detail.

jeff5and others added2 commitsAugust 29, 2022 23:45
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

You can dosetattr() in the same PR or in a separate PR linked to the same issue. Either way this won't be merged until someone from the SC agrees.

jeff5 reacted with thumbs up emoji
@gvanrossum
Copy link
Member

I would not do__dict__ yet, since that is currently pretty much defined to return adict, which doesn't easily let us require for string keys (except for the class dict, which is a readonly proxy guarded by the class setattr operation). I'd like this to remain strictly a docs change, not a code change.

jeff5 reacted with thumbs up emoji

@ChrisBarker-NOAA
Copy link
Contributor

ChrisBarker-NOAA commentedAug 30, 2022
edited
Loading

I would not do__dict__ yet, since that is currently pretty much defined to return a dict

Interesting -- so you can stuff pretty much whatever want into an object'sdict. However, if you do put a non-string key into adict, it will store it just fine, but not be accessible by getattr(), and only be accessible by directly accessing thedict.

I suppose it's possible that that could be a feature someone would (ab)use, but maybe the docs could advise against it, and/or call it unsupported?

@merwok
Copy link
Member

merwok commentedAug 30, 2022
edited
Loading

IMO it’s best for the docs to define the spec precisely, say something about strings that are not identifiers, but not also mention details of non-strings in__dict__ — so document how things work, but not all the ways in which things may not work.

@Gouvernathor
Copy link
Contributor

Wouldn't the safest option in short term be to precisely describe how the implementation works, and to categorize all of it as implementation details ? That way, programmers won't assume something is documented when it's just a behavior happening in their console.
Then, anything can always be decided later to be part of the spec (like dict ordering was) or dropped from the implementation. Is there precedent on this though ?

@gpshead
Copy link
Member

if you do put a non-string key into adict, it will store it just fine, but not be accessible by getattr(), and only be accessible by directly accessing thedict.

I suppose it's possible that that could be a feature someone would (ab)use, but maybe the docs could advise against it, and/or call it unsupported?

Sometimes documenting something that can be abused even if just to advise against it just leads to more abuse. So I'd lean towards not mentioning that you might be able to get away with stuffing a.__dict__ full of other unexpected key types.(There likely exists code that iterates an object dict and assumes the keys arestr that such a thing could trigger an exception out of).


Regardless, for this PR as is, documenting that** arguments allow any string keys, that seems reasonable to mention. 👍🏾

There are a pile of existing APIs in the world that take arbitrary keyword arguments and depend on this as a convenient feature. Many intentional, some maybe not wholly intentional but users who realize it enjoy the convenience anyways.

@jeff5
Copy link
ContributorAuthor

Wouldn't the safest option in short term be to precisely describe how the implementation works, and to categorize all of it as implementation details ?

@Gouvernathor This would be very much second best and not provide the consistency between implementations we hope for. The Language Reference aspires to document Python the language, and only occasionally (in grey boxes if we remember them) CPython the implementation.

@ChrisBarker-NOAA
Copy link
Contributor

I would not do__dict__ yet, since that is currently pretty much defined to return a dict

Now that I think about it -- it seems it would not be too hard to have a dict that only accepts string keys -- but yes, that's a code change, so a new topic and not part of this PR.

For the Typing folks: are the dunders often typed? that may be a place to define what a__dict__ is, even if not enforced.

@jeff5
Copy link
ContributorAuthor

jeff5 commentedSep 13, 2022
edited
Loading

this won't be merged until someone from the SC agrees.

Am I the one to request that? I would include#96454, but in a way that allowed a separate decision.

Is there a proper place (like the PEP repo) that puts it on the agenda. E-mail topython-dev or SC secretary maybe?

@merwok
Copy link
Member

Requests to the council are sent here:https://github.com/python/steering-council/issues/

@gvanrossum
Copy link
Member

Still waiting for the SC…

@jeff5
Copy link
ContributorAuthor

Still waiting for the SC…

Which I think this is waiting on me to ask it?

I can draft this weekend (if I can slip by the King's forces currently encamped around PyConUK).

@gvanrossum
Copy link
Member

Still waiting for the SC…

Which I think this is waiting on me to ask it?

I can draft this weekend (if I can slip by the King's forces currently encamped around PyConUK).

Thanks!

@jeff5
Copy link
ContributorAuthor

In view of the SC decision, can this (and#96454) now be merged?

@gvanrossumgvanrossum added needs backport to 3.10only security fixes needs backport to 3.11only security fixes and removed DO-NOT-MERGE labelsSep 22, 2022
@gvanrossumgvanrossum merged commit9d432b4 intopython:mainSep 22, 2022
@miss-islington
Copy link
Contributor

Thanks@jeff5 for the PR, and@gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-97025 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelSep 22, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestSep 22, 2022
…rs (pythonGH-96393)This represents the official SC stance, seehttps://github.com/python/steering-council/issues/142GH-issuecomment-1252172695(cherry picked from commit9d432b4)Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
@bedevere-bot
Copy link

GH-97026 is a backport of this pull request to the3.10 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.10only security fixes labelSep 22, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestSep 22, 2022
…rs (pythonGH-96393)This represents the official SC stance, seehttps://github.com/python/steering-council/issues/142GH-issuecomment-1252172695(cherry picked from commit9d432b4)Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
miss-islington added a commit that referenced this pull requestSep 22, 2022
…-96393)This represents the official SC stance, seehttps://github.com/python/steering-council/issues/142GH-issuecomment-1252172695(cherry picked from commit9d432b4)Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
miss-islington added a commit that referenced this pull requestSep 22, 2022
…-96393)This represents the official SC stance, seehttps://github.com/python/steering-council/issues/142GH-issuecomment-1252172695(cherry picked from commit9d432b4)Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
pablogsal pushed a commit that referenced this pull requestOct 22, 2022
…-96393)This represents the official SC stance, seehttps://github.com/python/steering-council/issues/142GH-issuecomment-1252172695(cherry picked from commit9d432b4)Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
@jeff5jeff5 deleted the doc-accept-nonid-kwargs branchFebruary 20, 2024 09:44
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gpsheadgpsheadgpshead left review comments

@pfmoorepfmoorepfmoore approved these changes

@gvanrossumgvanrossumgvanrossum approved these changes

@Yhg1sYhg1sAwaiting requested review from Yhg1s

Assignees

@gvanrossumgvanrossum

Labels
docsDocumentation in the Doc dirskip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clarify status of non-identifier unpacked keyword arguments
11 participants
@jeff5@merwok@gvanrossum@nascheme@ChrisBarker-NOAA@Gouvernathor@gpshead@miss-islington@bedevere-bot@pfmoore@AlexWaygood

[8]ページ先頭

©2009-2025 Movatter.jp