GeezerVison

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

GeezerVison is showing github.com — gh-103556: [inspect.Signature] disallow pos-or-kw params without default after pos-only with default by sobolevn · Pull Request #103557 · python/cpython

Conversation

sobolevn commented Apr 15, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

I consider this a bug fix, not a new feature. So, backport to 3.11 should probably be done.

Issue: Incorrect inspect.Signature can be created: pos-only with a default followed by pos-or-kw without one #103556

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-103556: [inspect.Signature] disallow pos-or-kw params withou…

578f998

…t default after pos-only with default

bedevere-bot mentioned this pull request

Apr 15, 2023

Incorrect inspect.Signature can be created: pos-only with a default followed by pos-or-kw without one #103556

Closed

bedevere-bot added the awaiting review label

Apr 15, 2023

sobolevn requested a review from carljm

April 15, 2023 09:47

sobolevn commented Apr 15, 2023

Copy link

Copy Markdown

Member Author

test_nntplib failure seems unrelated, but I cannot re-trigger Azure build.

All reactions

Sorry, something went wrong.

Uh oh!

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

sobolevn commented

Apr 15, 2023

View reviewed changes

Comment thread

Lib/test/test_inspect.py Resolved

Uh oh!

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


          Update Lib/test/test_inspect.py

a4cf538

sobolevn commented Apr 16, 2023

Copy link

Copy Markdown

Member Author

@hauntsaninja if you are interested in this as well after #103554 I would be very grateful :)

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm requested changes

Apr 17, 2023

View reviewed changes

carljm 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.

Thanks for finding and fixing this!

I think the fix can be even simpler, though; see inline comment.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/inspect.py Outdated

Comment on lines +3024 to +3029

		if (top_kind != _POSITIONAL_ONLY
		and kind != _POSITIONAL_OR_KEYWORD):
		# We still have to maintain defaults in cases like
		# def some(pod=42, /, pk=1): ...
		# Here `pk` must have a default value.
		kind_defaults = False

carljm Apr 17, 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.

non-default-after-default is only checked for _POSITIONAL_ONLY and _POSITIONAL_KEYWORD (see line 3032.) So if we want to keep the value of kind_defaults across the transition from PO to POK (which I do agree is correct and matches the parser behavior), then really we always want to keep it, and it is no longer kind_defaults but simply seen_default. So I think we can eliminate all of this, here's the full version I'd recommend (which passes all tests): https://gist.github.com/carljm/6779e601d04e0b1135444b4e0263e281

Sorry, something went wrong.

Uh oh!

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

All reactions

sobolevn Apr 19, 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.

Yes, I agree, thank you for the suggestion.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot removed the awaiting review label

Apr 17, 2023

bedevere-bot commented Apr 17, 2023

Copy link

Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added the awaiting changes label

Apr 17, 2023


          Address review

d6ef93a

sobolevn commented Apr 19, 2023

Copy link

Copy Markdown

Member Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

Apr 19, 2023

bedevere-bot commented Apr 19, 2023

Copy link

Copy Markdown

Thanks for making the requested changes!

@carljm: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot requested a review from carljm

April 19, 2023 08:40


          Merge branch 'issue-103556' of https://github.com/sobolevn/cpython in…

261d9d1

…to issue-103556

carljm approved these changes

Apr 22, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels

Apr 22, 2023

carljm added the needs backport to 3.11 only security fixes label

Apr 22, 2023

carljm merged commit 6b58d41 into python:main

Apr 22, 2023

bedevere-bot removed the awaiting merge label

Apr 22, 2023

miss-islington commented Apr 22, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Apr 22, 2023


          pythongh-103556: [inspect.Signature] disallow pos-or-kw params withou…

c38b670

…t default after pos-only with default (pythonGH-103557)

(cherry picked from commit 6b58d41)

Co-authored-by: Nikita Sobolev <[email protected]>

bedevere-bot commented Apr 22, 2023

Copy link

Copy Markdown

GH-103675 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the needs backport to 3.11 only security fixes label

Apr 22, 2023

carljm pushed a commit that referenced this pull request

Apr 22, 2023


          [3.11] gh-103556: [inspect.Signature] disallow pos-or-kw params witho…

b2fdae9

…ut default after pos-only with default (GH-103557) (#103675)

zhiruiluo mentioned this pull request

Jul 5, 2023

Fix test_decorator issue #269 lebrice/SimpleParsing#270

Closed

Reviewers

carljm

carljm approved these changes

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.

4 participants

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 01402520 — 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.

GeezerVison

Conversation

sobolevn commented Apr 15, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Apr 15, 2023

Uh oh!

Uh oh!

sobolevn commented Apr 16, 2023

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

sobolevn Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 17, 2023

Uh oh!

sobolevn commented Apr 19, 2023

Uh oh!

bedevere-bot commented Apr 19, 2023

Uh oh!

miss-islington commented Apr 22, 2023

Uh oh!

bedevere-bot commented Apr 22, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sobolevn commented Apr 15, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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