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-102433: Add tests for how classes with properties interact with `isinstance()` checks on `typing.runtime_checkable` protocols by AlexWaygood · Pull Request #102449 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Mar 5, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

It appears we currently have no tests for how classes with properties interact with isinstance 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 in python/typing#1363 is implemented.

  • Issue: isinstance on runtime_checkable Protocol has side-effects for @property methods #102433

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood
pythongh-102433: Add tests for how properties interact with `typing.r…
e32cde9 
…untime_checkable` protocols
@AlexWaygood AlexWaygood added skip news needs backport to 3.10 needs backport to 3.11 only security fixes labels Mar 5, 2023
@AlexWaygood
AlexWaygood requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners March 5, 2023 18:31
@bedevere-bot bedevere-bot added the tests Tests in the Lib/test dir label Mar 5, 2023
@bedevere-bot bedevere-bot mentioned this pull request Mar 5, 2023
isinstance on runtime_checkable Protocol has side-effects for @property methods #102433
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Mar 5, 2023
@AlexWaygood
AlexWaygood requested a review from sobolevn March 5, 2023 18:32
JelleZijlstra
JelleZijlstra reviewed Mar 6, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood added the DO-NOT-MERGE label Mar 6, 2023
@AlexWaygood

AlexWaygood commented Mar 6, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
AlexWaygood marked this pull request as draft March 6, 2023 11:29
@AlexWaygood
Don't assert undesirable behaviour; cover other situations too
74eb795
@AlexWaygood
AlexWaygood marked this pull request as ready for review March 10, 2023 17:06
@AlexWaygood AlexWaygood removed the DO-NOT-MERGE label Mar 10, 2023
@AlexWaygood

AlexWaygood commented Mar 10, 2023

Copy link
Copy Markdown
Member Author

I've removed the tests that assert undesirable behaviour, as per the consensus in python/typing#1363.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
AlexWaygood requested a review from carljm March 10, 2023 17:07
AlexWaygood
AlexWaygood commented Mar 10, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Update Lib/test/test_typing.py
b4ed977
@AlexWaygood
AlexWaygood requested a review from JelleZijlstra March 10, 2023 17:09
@AlexWaygood
nit
14268e5
sobolevn
sobolevn reviewed Mar 10, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_typing.py
class C:
@property
def attr(self):
return 42

@sobolevn sobolevn Mar 10, 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 about side-effects in @property like raise ValueError? Should we test this case?

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood AlexWaygood Mar 10, 2023 •
edited
Loading

Uh oh!

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

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.

We should test this case, definitely! But the behaviour for this case may be about to change if any of the patches discussed in python/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.

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Address review
f2bc540
@AlexWaygood
AlexWaygood requested a review from sobolevn March 10, 2023 23:36
carljm
carljm approved these changes Mar 11, 2023
View reviewed changes

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

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

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
Comment thread Lib/test/test_typing.py
class BadPG1(Protocol[T]):
attr: T

for obj in PG[T], PG[C], PG1[T], PG1[C], BadP, BadP1, BadPG, BadPG1:

@carljm carljm Mar 11, 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.

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

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood AlexWaygood Mar 11, 2023 •
edited
Loading

Uh oh!

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

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Mar 11, 2023
@AlexWaygood @carljm
Update Lib/test/test_typing.py
ba178f1 
Co-authored-by: Carl Meyer <[email protected]>
@AlexWaygood
AlexWaygood merged commit 5ffdaf7 into python:main Mar 11, 2023
@miss-islington

miss-islington commented Mar 11, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 11, 2023
@AlexWaygood
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
@AlexWaygood @carljm @miss-islington
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 commit 5ffdaf7)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Carl Meyer <[email protected]>
@bedevere-bot

bedevere-bot commented Mar 11, 2023

Copy link
Copy Markdown

GH-102592 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 bedevere-bot removed the needs backport to 3.11 only security fixes label Mar 11, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2023
@AlexWaygood @carljm @miss-islington
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 commit 5ffdaf7)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Carl Meyer <[email protected]>
@bedevere-bot

bedevere-bot commented Mar 11, 2023

Copy link
Copy Markdown

GH-102593 is a backport of this pull request to the 3.10 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Mar 11, 2023
@AlexWaygood

AlexWaygood commented Mar 11, 2023

Copy link
Copy Markdown
Member Author

Thanks, all!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Mar 11, 2023
@miss-islington @AlexWaygood @carljm
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 commit 5ffdaf7)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Carl Meyer <[email protected]>
miss-islington added a commit that referenced this pull request Mar 11, 2023
@miss-islington @AlexWaygood @carljm
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 commit 5ffdaf7)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Carl Meyer <[email protected]>
iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request Mar 12, 2023
@AlexWaygood @carljm @iritkatriel
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 <[email protected]>
@AlexWaygood AlexWaygood mentioned this pull request Apr 12, 2023
Backport test coverage improvements for runtime-checkable protocols python/typing_extensions#136
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner
@sobolevn sobolevn Awaiting requested review from sobolevn

Uh oh!

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

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

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.

6 participants

@AlexWaygood @miss-islington @bedevere-bot @carljm @JelleZijlstra @sobolevn
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.