Skip to content

Conversation

@altendky
Copy link
Contributor

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

@altendky altendky requested a review from a team as a code owner April 26, 2023 18:50
@altendky altendky added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels Apr 26, 2023
@altendky altendky requested a review from xdustinface April 26, 2023 19:00
@arvidn
Copy link
Contributor

arvidn commented Apr 26, 2023

what's the behavior of moving those checks into the class body itself?

@altendky
Copy link
Contributor Author

It makes them located closer to the class code where I suspect people would expect it to be. This should make the pattern more apparent to people (sorta...) and avoid stuff like #14991. In terms of functional change, you get a class var instead of a global (not a big deal) and the cast() pattern avoids 'instantiating' the class which makes it more flexible for use with classes that require parameters to be instantiated.

Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

you get a class var instead of a global

Do we actually get the class var in there since it's only happening if TYPE_CHECKING?

@altendky
Copy link
Contributor Author

If you're type checking you do. ;] Which may include IDEs, I'm not sure.

@wallentx wallentx merged commit 322c54f into main Apr 27, 2023
@wallentx wallentx deleted the rework_class_protocol_check_pattern branch April 27, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants