GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-107704: Argument Clinic: add support for deprecating keyword use of parameters by serhiy-storchaka · Pull Request #107984 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Aug 15, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

It is now possible to deprecate passing keyword arguments for keyword-or-positional parameters with Argument Clinic, using the new '/ [from X.Y]' syntax.
(To be read as "positional-only from Python version X.Y")

  • Issue: Argument Clinic: add support for deprecating keyword use of parameters #107704

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji erlend-aasland reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 1 reaction
@serhiy-storchaka
serhiy-storchaka requested review from AlexWaygood and erlend-aasland August 15, 2023 17:33
@serhiy-storchaka
pythongh-107704: Argument Clinic: add support for deprecating keyword…
059cd6a 
… use of parameters

It is now possible to deprecate passing keyword arguments for
keyword-or-positional parameters with Argument Clinic, using the new
'/ [from X.Y]' syntax.
(To be read as "positional-only from Python version X.Y")
@serhiy-storchaka
serhiy-storchaka force-pushed the clinic-deprecated-keyword branch from 2b3c1e2 to 059cd6a Compare August 15, 2023 17:37
@bedevere-bot bedevere-bot mentioned this pull request Aug 15, 2023
Argument Clinic: add support for deprecating keyword use of parameters #107704
Closed
serhiy-storchaka added 2 commits August 15, 2023 21:22
@serhiy-storchaka
Silence check-c-globals.py.
b4b9f3c
@serhiy-storchaka
Update the documentation.
d5e0064
@erlend-aasland

erlend-aasland commented Aug 15, 2023

Copy link
Copy Markdown
Contributor

Do you want an initial review of the draft, or would you like to wait until it is marked ready for review?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Make constructor's arguments optional, it produces more interesting o…
b1300c7 
…utput.
@serhiy-storchaka
serhiy-storchaka marked this pull request as ready for review August 15, 2023 19:38
@serhiy-storchaka
serhiy-storchaka requested a review from berkerpeksag as a code owner August 15, 2023 19:38
@serhiy-storchaka

serhiy-storchaka commented Aug 15, 2023

Copy link
Copy Markdown
Member Author

I kept it in a draft state for the first run of tests and the final stage of self-review.

It is now ready for review.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting core review label Aug 15, 2023
erlend-aasland
erlend-aasland reviewed Aug 16, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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 think it would be beneficial to try and reduce some of the code duplication, either in this PR, or in a following-up refactor. Perhaps the latter is preferable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/howto/clinic.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Outdated
Comment on lines +1001 to +1006
if func.kind.new_or_init:
conditions.append(f"nargs < {i+1} && kwargs && PyDict_Contains(kwargs, &_Py_ID({p.name}))")
containscheck = "PyDict_Contains"
else:
conditions.append(f"nargs < {i+1} && kwnames && PySequence_Contains(kwnames, &_Py_ID({p.name}))")
containscheck = "PySequence_Contains"

@erlend-aasland erlend-aasland Aug 16, 2023

Copy link
Copy Markdown
Contributor

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.

Regarding _Py_ID:

  • See commit 6f6a4e6
  • gh-90928: Statically Initialize the Keywords Tuple in Clinic-Generated Code #95860
  • Statically Initialize PyArg_Parser in clinic.py #90928
  • gh-90928: Improve static initialization of keywords tuple in AC #95907

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 16, 2023

Copy link
Copy Markdown
Member Author

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.

Unfortunately it is the simplest general way to check that the argument was passed by keyword. It is not the fastest. For inlined parsing code we can use more efficient but more complex code, we can even build it in _PyArg_UnpackKeywords(). But if PyArg_ParseTupleAndKeywords() or similar function is used, the only other way is to use deprecated and inefficient PyMapping_HasKeyString() or introduce a new private inefficient API.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland Aug 16, 2023

Copy link
Copy Markdown
Contributor

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.

cc. @ericsnowcurrently

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Aug 16, 2023

Copy link
Copy Markdown
Member

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.

What's the concern with using _Py_ID() here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland Aug 17, 2023

Copy link
Copy Markdown
Contributor

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.

Currently, there is no concern; I just wanted to point out that even though Argument Clinic does not currently care about the Limited API, we may want to add such functionality in the future; IMO, it should be possible for Argument Clinic to generate code that does not depend on internal APIs, for example for stdlib extension modules such as sqlite3.

Many core devs seem to be in favour of not using internal APIs in stdlib extension modules, but I'm not sure if there is a consensus.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Address review comments.
ad36c15
serhiy-storchaka
serhiy-storchaka commented Aug 16, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

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 for your review @erlend-aasland. I tried to address them. Seems the only unresolved issue is with _Py_ID().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/howto/clinic.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Aug 16, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka added 2 commits August 17, 2023 13:31
@serhiy-storchaka
Add a test for * [from ...] with not inlined parsing code
e271834
@serhiy-storchaka
Consistently use simple function name in error messages.
e603de7
erlend-aasland
erlend-aasland approved these changes Aug 18, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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 doing this, @serhiy-storchaka. @AlexWaygood, do you want to have a look?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 18, 2023
AlexWaygood
AlexWaygood reviewed Aug 18, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

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.

Some small suggestions for the docs:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/howto/clinic.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/howto/clinic.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/howto/clinic.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood
AlexWaygood reviewed Aug 18, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

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.

Code looks good, just a few very minor nits:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka commented Aug 18, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

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 for your review @AlexWaygood. Do these docstrings work?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka and others added 3 commits August 18, 2023 16:05
@serhiy-storchaka @AlexWaygood
Apply suggestions from code review
3b6f6c4 
Co-authored-by: Alex Waygood <[email protected]>
@serhiy-storchaka
Merge branch 'main' into clinic-deprecated-keyword
94d9b34
@serhiy-storchaka
Rename _testclinic_depr_star.c.h -> _testclinic_depr.c.h
158c5e3
AlexWaygood
AlexWaygood approved these changes Aug 18, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

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.

LGTM. Thanks @serhiy-storchaka!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
More efficient checks in case of inlined parsing code.
5c92d36
serhiy-storchaka added 3 commits August 18, 2023 22:20
@serhiy-storchaka
Fix non-inlined parsing in __init__ and __new__.
6607137
@serhiy-storchaka
Fix a case of: inlined parsing + no keyword args + omit some some opt…
9fee851 
…ional args.
@serhiy-storchaka
Update Tools/c-analyzer/cpython/globals-to-fix.tsv.
38c9560
@serhiy-storchaka

serhiy-storchaka commented Aug 19, 2023

Copy link
Copy Markdown
Member Author

Post-review changes:

  • I have found how to make keyword argument check more efficient in case of inlined parsing code. It now has almost zero overhead.
  • Since the tests with the new generated code no longer covered all cases in the generator, I added new tests, and they exposed bugs in a corner case in constructors (including * [from ...]). Fixed.
erlend-aasland reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Merge branch 'main' into clinic-deprecated-keyword
71ea31c
@serhiy-storchaka
serhiy-storchaka merged commit 2f31143 into python:main Aug 19, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 19, 2023
@serhiy-storchaka
serhiy-storchaka deleted the clinic-deprecated-keyword branch August 19, 2023 07:13
AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Sep 26, 2023
@serhiy-storchaka @erlend-aasland @AlexWaygood
python/cpython#107704: Argument Clinic: add support for deprecating k…
3ab432c 
…eyword use of parameters (python/cpython#107984)

It is now possible to deprecate passing keyword arguments for
keyword-or-positional parameters with Argument Clinic, using the new
'/ [from X.Y]' syntax.
(To be read as "positional-only from Python version X.Y")

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
erlend-aasland added a commit to python/devguide that referenced this pull request Oct 1, 2023
@serhiy-storchaka @erlend-aasland @AlexWaygood
python/cpython#107704: Argument Clinic: add support for deprecating k…
30dfd7c 
…eyword use of parameters (python/cpython#107984)

It is now possible to deprecate passing keyword arguments for
keyword-or-positional parameters with Argument Clinic, using the new
'/ [from X.Y]' syntax.
(To be read as "positional-only from Python version X.Y")

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
@erlend-aasland erlend-aasland mentioned this pull request Apr 2, 2024
Speed up s.startswith() faster-cpython/ideas#671
Open
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
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@AlexWaygood AlexWaygood AlexWaygood approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@serhiy-storchaka @erlend-aasland @ericsnowcurrently @AlexWaygood @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.