NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

gh-102433: Add tests for how classes with properties interact with`isinstance()` checks on`typing.runtime_checkable` protocols#102449

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

Jump to bottom

Merged

AlexWaygood merged 6 commits intopython:mainfromAlexWaygood:runtime-checkable-tests

Mar 11, 2023

Merged

gh-102433: Add tests for how classes with properties interact with`isinstance()` checks on`typing.runtime_checkable` protocols#102449

AlexWaygood merged 6 commits intopython:mainfromAlexWaygood:runtime-checkable-tests

Mar 11, 2023

Conversation

Copy link

Member

AlexWaygood commentedMar 5, 2023•
edited
Loading

It appears we currently have no tests for how classes with properties interact withisinstance checks for protocols decorated with@runtime_checkable.

I'm trying to only add tests here for uncontroversial behaviour that we won't want changed, whether or not any of the patches discussed inpython/typing#1363 is implemented.

Issue:isinstance onruntime_checkableProtocol has side-effects for@property methods #102433

pythongh-102433: Add tests for how properties interact with `typing.r…

e32cde9

…untime_checkable` protocols

AlexWaygood added skip news needs backport to 3.10

only security fixes

needs backport to 3.11only security fixes labels

Mar 5, 2023

AlexWaygood requested review fromFidget-Spinner,JelleZijlstra andgvanrossum ascode owners

March 5, 2023 18:31

bedevere-bot added the testsTests in the Lib/test dir label

Mar 5, 2023

bedevere-bot mentioned this pull request

Mar 5, 2023

isinstance onruntime_checkableProtocol has side-effects for@property methods#102433

Closed

bedevere-bot added the awaiting core review label

Mar 5, 2023

AlexWaygood requested a review fromsobolevn

March 5, 2023 18:32

JelleZijlstra reviewed

Mar 6, 2023

View reviewed changes

Lib/test/test_typing.py OutdatedShow resolvedHide resolved

AlexWaygood added the DO-NOT-MERGE label

Mar 6, 2023

Copy link

MemberAuthor

AlexWaygood commentedMar 6, 2023

Marking as "DO-NOT-MERGE" for now, as this appears more controversial than I realised.

AlexWaygood marked this pull request as draft

March 6, 2023 11:29

Don't assert undesirable behaviour; cover other situations too

74eb795

AlexWaygood marked this pull request as ready for review

March 10, 2023 17:06

AlexWaygood removed the DO-NOT-MERGE label

Mar 10, 2023

Copy link

MemberAuthor

AlexWaygood commentedMar 10, 2023

I've removed the tests that assert undesirable behaviour, as per the consensus inpython/typing#1363.

AlexWaygood requested a review fromcarljm

March 10, 2023 17:07

AlexWaygood commented

Mar 10, 2023

View reviewed changes

Lib/test/test_typing.py OutdatedShow resolvedHide resolved

Update Lib/test/test_typing.py

b4ed977

AlexWaygood requested a review fromJelleZijlstra

March 10, 2023 17:09

nit

14268e5

sobolevn reviewed

Mar 10, 2023

View reviewed changes

Lib/test/test_typing.py OutdatedShow resolvedHide resolved

Lib/test/test_typing.py

		classC:
		@property
		defattr(self):
		return42

Copy link

Member

sobolevnMar 10, 2023

Choose a reason for hiding this comment

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

What about side-effects in@property likeraise ValueError? Should we test this case?

Copy link

MemberAuthor

AlexWaygoodMar 10, 2023•
edited
Loading

Choose a reason for hiding this comment

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

We should test this case, definitely! But the behaviour for this case may be about to change if any of the patches discussed inpython/typing#1363 is implemented (and the consensus seems to be that we should implement one of those patches). If so, we should add the tests in the same PR as we change the behaviour.

Whether or not we decide to change the behaviour around properties that have side effects, I'd prefer to add those tests in a separate PR, so that this PR is solely focussed on adding tests for uncontroversial behaviour.

Lib/test/test_typing.py OutdatedShow resolvedHide resolved

Address review

f2bc540

AlexWaygood requested a review fromsobolevn

March 10, 2023 23:36

carljm approved these changes

Mar 11, 2023

View reviewed changes

Copy link

Member

carljm left a comment

Choose a reason for hiding this comment

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

A couple nits, but these tests look good to me! Thank you!

Lib/test/test_typing.py

		classBadPG1(Protocol[T]):
		attr:T

		forobjinPG[T],PG[C],PG1[T],PG1[C],BadP,BadP1,BadPG,BadPG1:

Copy link

Member

carljmMar 11, 2023

Choose a reason for hiding this comment

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

nit: these are all protocol classes still; the name'protocol_class used in the above "good" loop seems much clearer than the nameobj

Copy link

MemberAuthor

AlexWaygoodMar 11, 2023•
edited
Loading

Choose a reason for hiding this comment

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

No strong opinion here, but the reason I avoided "protocol_class" for these ones is that the parameterised ones (PG[T], etc) aren'tstrictly speaking classes anymore — they're generic aliases to protocol classes.

Lib/test/test_typing.py OutdatedShow resolvedHide resolved

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

Mar 11, 2023

Update Lib/test/test_typing.py

ba178f1

Co-authored-by: Carl Meyer <carl@oddbird.net>

AlexWaygood merged commit5ffdaf7 intopython:main

Mar 11, 2023

Copy link

Contributor

miss-islington commentedMar 11, 2023

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

bedevere-bot removed the awaiting merge label

Mar 11, 2023

AlexWaygood deleted the runtime-checkable-tests branch

March 11, 2023 01:20

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

Mar 11, 2023

pythongh-102433: Add tests for how classes with properties interact w…

6c77697

…ith `isinstance()` checks on `typing.runtime_checkable` protocols (pythonGH-102449)(cherry picked from commit5ffdaf7)Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Carl Meyer <carl@oddbird.net>

Copy link

bedevere-bot commentedMar 11, 2023

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

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

Mar 11, 2023

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

Mar 11, 2023

pythongh-102433: Add tests for how classes with properties interact w…

385fae3

…ith `isinstance()` checks on `typing.runtime_checkable` protocols (pythonGH-102449)(cherry picked from commit5ffdaf7)Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Carl Meyer <carl@oddbird.net>

Copy link

bedevere-bot commentedMar 11, 2023

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

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

Mar 11, 2023

Copy link

MemberAuthor

AlexWaygood commentedMar 11, 2023

Thanks, all!

miss-islington added a commit that referenced this pull request

Mar 11, 2023

gh-102433: Add tests for how classes with properties interact with `i…

2810787

…sinstance()` checks on `typing.runtime_checkable` protocols (GH-102449)(cherry picked from commit5ffdaf7)Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Carl Meyer <carl@oddbird.net>

miss-islington added a commit that referenced this pull request

Mar 11, 2023

gh-102433: Add tests for how classes with properties interact with `i…

e467cb3

…sinstance()` checks on `typing.runtime_checkable` protocols (GH-102449)(cherry picked from commit5ffdaf7)Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Carl Meyer <carl@oddbird.net>

iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request

Mar 12, 2023

pythongh-102433: Add tests for how classes with properties interact w…

fde1a99

…ith `isinstance()` checks on `typing.runtime_checkable` protocols (python#102449)Co-authored-by: Carl Meyer <carl@oddbird.net>

AlexWaygood mentioned this pull request

Apr 12, 2023

Backport test coverage improvements for runtime-checkable protocolspython/typing_extensions#136

Merged

Labels

skip news tests

Tests in the Lib/test dir

Movatterモバイル変換

Uh oh!

gh-102433: Add tests for how classes with properties interact withisinstance() checks ontyping.runtime_checkable protocols#102449

gh-102433: Add tests for how classes with properties interact withisinstance() checks ontyping.runtime_checkable protocols#102449

Uh oh!

Conversation

AlexWaygood commentedMar 5, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commentedMar 6, 2023

Uh oh!

AlexWaygood commentedMar 10, 2023

Uh oh!

Uh oh!

Uh oh!

sobolevnMar 10, 2023

Choose a reason for hiding this comment

Uh oh!

AlexWaygoodMar 10, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljmMar 11, 2023

Choose a reason for hiding this comment

Uh oh!

AlexWaygoodMar 11, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington commentedMar 11, 2023

Uh oh!

bedevere-bot commentedMar 11, 2023

Uh oh!

bedevere-bot commentedMar 11, 2023

Uh oh!

AlexWaygood commentedMar 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gh-102433: Add tests for how classes with properties interact with`isinstance()` checks on`typing.runtime_checkable` protocols#102449

gh-102433: Add tests for how classes with properties interact with`isinstance()` checks on`typing.runtime_checkable` protocols#102449

AlexWaygood commentedMar 5, 2023•
edited
Loading

AlexWaygoodMar 10, 2023•
edited
Loading

AlexWaygoodMar 11, 2023•
edited
Loading