GeezerVison

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

GeezerVison is showing github.com — gh-104050: Add type annotations to sentinels in Argument Clinic by erlend-aasland · Pull Request #104589 · python/cpython

Conversation

erlend-aasland commented May 17, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Issue: Add type annotations to clinic.py #104050

Sorry, something went wrong.

Uh oh!

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

AlexWaygood reacted with hooray emoji

All reactions

🎉 1 reaction


          pythongh-104050: Add type annotations to sentinels in Argument Clinic

3b12493

erlend-aasland added the skip news label

May 17, 2023

erlend-aasland requested a review from AlexWaygood

May 17, 2023 20:31

bedevere-bot mentioned this pull request

May 17, 2023

Add type annotations to clinic.py #104050

Closed

erlend-aasland commented May 17, 2023

Copy link

Copy Markdown

Contributor Author

I get complaints about isinstance checks now. Using type(sentinel) works, but feel insanely clumsy. Surely there must be a better way to do this.

$ mypy Tools/clinic
Tools/clinic/clinic.py:2697: error: Name "Unknown" is not defined  [name-defined]
Tools/clinic/clinic.py:3478: error: Name "Null" is not defined  [name-defined]
Tools/clinic/clinic.py:3632: error: Name "Null" is not defined  [name-defined]
Tools/clinic/clinic.py:3656: error: Name "Null" is not defined  [name-defined]
Found 4 errors in 1 file (checked 2 source files)

All reactions

Sorry, something went wrong.

Uh oh!

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

AlexWaygood commented May 17, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Rather than doing if isinstance(x, Unknown) as the test, you can do if x is unknown. Since unknown is a sentinel value, the identity check is idiomatic.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland commented May 17, 2023

Copy link

Copy Markdown

Contributor Author

Rather than doing if isinstance(x, Unknown), you can do if x is unknown. Since unknown is a sentinel value, the identity check is idiomatic.

Doh. Thanks!

AlexWaygood reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

AlexWaygood commented May 17, 2023

Copy link

Copy Markdown

Member

The usage of NULL seems a little different from the other two, though, so maybe that one does need its own class. (Not sure about that -- I'm unfamiliar with this code!)

E.g. I can't say I fully understand what's going on with the Null usage here:

cpython/Tools/clinic/clinic.py

Line 3483 in aed643b

default_type = (str, Null, NoneType)

All reactions

Sorry, something went wrong.

Uh oh!

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


          Merge branch 'main' into typed-clinic/sentinels

5c1c2df

erlend-aasland commented May 18, 2023

Copy link

Copy Markdown

Contributor Author

Yeah, the NULL usages need a different treatment. I'll have a look later today.

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


          Fix stuff

d145ff8

- Fix variable names
- Add NullType
- Fix instance check

erlend-aasland marked this pull request as ready for review

May 18, 2023 19:44

bedevere-bot added the awaiting core review label

May 18, 2023


          Revert unneeded change

This comment was marked as outdated.

AlexWaygood reviewed

May 18, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py

		NULL: Final = Sentinels.NULL
		unknown: Final = Sentinels.unknown

		NullType = type(Sentinels.NULL)

AlexWaygood May 18, 2023 •

edited

Loading

Uh oh!

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

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.

Hmm, The type of Sentinels.Null is just Sentinels. For Python enums, all enum members are instances of the enum class. (This is why I was wondering if maybe the Null class should just be left as it is; it maybe needs to be its own class, rather than sharing the same class as the other sentinel values.)

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland May 18, 2023

Copy link

Copy Markdown

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

Oh, you're right.

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland May 18, 2023

Copy link

Copy Markdown

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

Perhaps we should amend this instead:

cpython/Tools/clinic/clinic.py

Lines 2605 to 2607 in dcdc90d

	# If not None, default must be isinstance() of this type.
	# (You can also specify a tuple of types.)
	default_type: bltns.type[Any] \| tuple[bltns.type[Any], ...] \| None = None

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland May 18, 2023

Copy link

Copy Markdown

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

I think my approach is fundamentally flawed. This is pretty simple really: we simply want distinct (sentinel) types for default_type and the instance check (🥁).

Sorry, something went wrong.

Uh oh!

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

All reactions

AlexWaygood May 18, 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.

I'm not sure if we're in agreement or not (possibly my brain's going fuzzy at the end of a long day 😆), but I think for this PR, I might keep the refactor for unspecified and unknown, but leave Null as it is on main.

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with laugh emoji

All reactions

😄 1 reaction

AlexWaygood May 18, 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.

Fab, thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland May 18, 2023

Copy link

Copy Markdown

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

The annotation for default_type is probably also incorrect.

cpython/Tools/clinic/clinic.py

Lines 2598 to 2607 in dcdc90d

	# The Python default value for this parameter, as a Python value.
	# Or the magic value "unspecified" if there is no default.
	# Or the magic value "unknown" if this value is a cannot be evaluated
	# at Argument-Clinic-preprocessing time (but is presumed to be valid
	# at runtime).
	default: bool \| Unspecified = unspecified

	# If not None, default must be isinstance() of this type.
	# (You can also specify a tuple of types.)
	default_type: bltns.type[Any] \| tuple[bltns.type[Any], ...] \| None = None

Sorry, something went wrong.

Uh oh!

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

All reactions

AlexWaygood May 18, 2023 •

edited

Loading

Uh oh!

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

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.

The annotation for default_type is probably also incorrect.

It looks correct to me based on the comment and usage. But maybe I'm not seeing something. What makes you think it's incorrect?

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland May 18, 2023

Copy link

Copy Markdown

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

With a custom converter, you can override pretty much anything (AFAIK), so I think bltns.type[Any] is too narrow. I might be wrong.

Sorry, something went wrong.

Uh oh!

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

All reactions

AlexWaygood May 18, 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.

The comment indicates that default has to be an instance of default_type if default_type is not None, and isintance(x, Foo) for any given x will fail if Foo is not an instance of type or a tuple of objects which are instances of type

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction


          Fix local rebase and write nicer if for line 2700

703c417

erlend-aasland closed this

May 18, 2023

erlend-aasland deleted the typed-clinic/sentinels branch

May 18, 2023 20:24

Reviewers

AlexWaygood

AlexWaygood left review comments

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review skip news

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.

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

Conversation

erlend-aasland commented May 17, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 17, 2023

Uh oh!

AlexWaygood commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 17, 2023

Uh oh!

AlexWaygood commented May 17, 2023

Uh oh!

erlend-aasland commented May 18, 2023

Uh oh!

This comment was marked as outdated.

AlexWaygood May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erlend-aasland commented May 17, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

AlexWaygood commented May 17, 2023 •

edited

Loading

Uh oh!

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

AlexWaygood May 18, 2023 •

edited

Loading

Uh oh!

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

AlexWaygood May 18, 2023 •

edited

Loading

Uh oh!

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