-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
PEP 736: Shorthand syntax for keyword arguments at invocation #3551
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for the PR! You've marked it as a draft, is it ready for review? |
Moving this to 'Ready for review' now. |
hugovk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with the PEP-NNNN template, should we add these missing sections?
For things like Backwards Compatibility and Security Implications, if there's no concerns, it can be useful to say so explicitly. For example, compare https://peps.python.org/pep-0737/#backwards-compatibility
I think How to Teach This would be a useful addition. Probably fine if there's no Reference Implementation.
Backwards Compatibility
=======================
[Describe potential impact and severity on pre-existing code.]
Security Implications
=====================
[How could a malicious user take advantage of this new feature?]
How to Teach This
=================
[How to teach users, new and experienced, how to apply the PEP to their work.]
Reference Implementation
========================
[Link to any existing implementation and details about its state, e.g. proof-of-concept.]| Content-Type: text/x-rst | ||
| Created: 28-Nov-2023 | ||
| Python-Version: 3.13 | ||
| Post-History: 14-Oct-2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need linking to the new discussion, once the PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry which new discussion are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply, I missed this at the time.
https://peps.python.org/pep-0001/#discussing-a-PEP says:
As soon as a PEP number has been assigned and the draft PEP is committed to the PEP repository, a discussion thread for the PEP should be created to provide a central place to discuss and review its contents, and the PEP should be updated so that the Discussions-To header links to it.
...
If a PEP undergoes a significant re-write or other major, substantive changes to its proposed specification, a new thread should typically be created in the chosen venue to solicit additional feedback. If this occurs, the Discussions-To link must be updated and a new Post-History entry added pointing to this new thread.
Good point, I will add those.
I'll have a think about this and look at some other examples.
I've added a reference implementation section with a link. However the link target should probably become PR rather than a branch. |
b471b1a to
177ef6a
Compare
| ========== | ||
|
|
||
| .. [1] Issue 36817: Add = to f-strings for easier debugging. - Python tracker | ||
| https://bugs.python.org/issue36817 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub is more accessible than bpo:
| https://bugs.python.org/issue36817 | |
| https://github.com/python/cpython/issues/80998 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will update
| .. [1] Issue 36817: Add = to f-strings for easier debugging. - Python tracker | ||
| https://bugs.python.org/issue36817 | ||
| .. [2] Ruby keyword argument syntactic sugar | ||
| https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/#:~:text=Other%20Notable%20New%20Features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these highlights intentional? If not, let's delete. (I'm not sure if they're Chrome only?)
| https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/#:~:text=Other%20Notable%20New%20Features | |
| https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is intentional. It looks to me like it has broad browser support as well:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using Firefox, and the highlight didn't do anything for me. It might be better to deep link to the nearest linkable section heading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, caniuse.com indicates that unfortunately Firefox is the only major browser lacking support yet. The ruby-lang.org doesn't have any IDs I can anchor to so my sense is to leave this one is as it's helpful where it does work? For reasonml.github.io, I'll switch to #function-application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pages without linkable anchors, I tend to add a hint in the reference on what word or words to search for in the page, like this: Ruby 3.1.0 release notes (search for "keyword arguments").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea, will do.
| .. [2] Ruby keyword argument syntactic sugar | ||
| https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/#:~:text=Other%20Notable%20New%20Features | ||
| .. [3] ReasonML named argument punning | ||
| https://reasonml.github.io/docs/en/function#:~:text=Named%20argument%20punning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| https://reasonml.github.io/docs/en/function#:~:text=Named%20argument%20punning | |
| https://reasonml.github.io/docs/en/function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this was intentional. Happy to remove if there's a hard preference but I think the highlights add value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will switch to #function-application for Firefox support.
|
@joshuabambrick @Rosuav Where are we up to with this? Are you ready for merge? |
|
I'm happy to see it merged and ready for the next round of discussion. |
|
OK, let's merge! We have to click "resolve" on the suggestions above to merge, but they can wait for next time. |
|
Thanks all for your input on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added 3 comments, let me know if I shouldn't review here directly and rather comment in the Discuss thread 😉
EDIT: oh, I realized just now it's already merged... 😫
| The feature is confusing | ||
| ------------------------ | ||
|
|
||
| We argue that: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be explained why the feature is deemed confusing by some? Only listing counter-points in favor of the PEP seems unfair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: oh, I realized just now it's already merged...
No worries, I appreciate your comments and will certainly consider them for the next set of edits.
I don't recall coming across any reasons being offered for why the feature may be confusing; I suspect it is primarily the fact that it is new syntax to this language and not that there is anything intrinsically confusing about this syntax in particular. That said, if you identify any concrete reasons, please do let me know on the Discuss thread.
| if not, then an arbitrary grouping is enforced between different types of | ||
| keyword arguments. | ||
|
|
||
| Objections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to address the impact on IDEs. There's a comment in the thread that asks, what happens when you ctrl-click name=? With name=name, you can both go to the name parameter signature and the name variable. What should/could IDEs do with only name=?
Same question for API documentation tools, though I didn't see it in the thread so will expand there myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice to see notes about the impact it could have on code. The thread mentions the influence it could have on renaming or re-assigning variables to be able to use the feature:
-call(pos, depth=depth+1, opts=options)
+depth += 1
+opts = options
+call(pos, depth=, opts=)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I've try to address these suggestions in #3639
| Author: Joshua Bambrick <jbambrick@google.com>, | ||
| Chris Angelico <rosuav@gmail.com> | ||
| Sponsor: Guido van Rossum <guido@python.org> | ||
| Discussions-To: https://discuss.python.org/t/syntactic-sugar-to-encourage-use-of-named-arguments/36217 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the new thread. Please can you update the PEP?
| Discussions-To: https://discuss.python.org/t/syntactic-sugar-to-encourage-use-of-named-arguments/36217 | |
| Discussions-To: https://discuss.python.org/t/pep-736-shorthand-syntax-for-keyword-arguments-at-invocation/43432 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Yes will do. I'm working on addressing the comments here joshuabambrick#3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Please could you make a PR just to update this and Post-History? It's been 6 weeks now, anyone reading through the PEP may only know about the old discussion thread and miss the new one. Thanks!
| Type: Standards Track | ||
| Created: 28-Nov-2023 | ||
| Python-Version: 3.13 | ||
| Post-History: 14-Oct-2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here:
| Post-History: 14-Oct-2023 | |
| Post-History: `17-Jan-2024 <https://discuss.python.org/t/pep-736-shorthand-syntax-for-keyword-arguments-at-invocation/43432>`__ |
Basic requirements (all PEP Types)
pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) andPEPheaderAuthororSponsor, and formally confirmed their approvalAuthor,Status(Draft),TypeandCreatedheaders filled out correctlyPEP-Delegate,Topic,RequiresandReplacesheaders completed if appropriate.github/CODEOWNERSfor the PEPStandards Track requirements
Python-Versionset to valid (pre-beta) future Python version, if relevantDiscussions-ToandPost-History📚 Documentation preview 📚: https://pep-previews--3551.org.readthedocs.build/pep-0736/