Skip to content

GH-103721: Allow defining GenericAlias-like things #103722

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Apr 23, 2023

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Two PEP8 nits:

adriangb and others added 2 commits April 23, 2023 13:11
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@arhadthedev arhadthedev added stdlib Python modules in the Lib dir topic-typing labels Apr 23, 2023
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Two more small comments below (wrap to 80 chars for .py and .rst files, use Sphinx markup to link to the rest of the docs).

In general, I think this is a reasonable feature to implement, but I think it should probably be documented somewhere in typing.rst.

@AlexWaygood AlexWaygood added type-feature A feature request or enhancement 3.12 only security fixes labels Apr 23, 2023
adriangb and others added 2 commits April 23, 2023 13:17
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
…Ve-P9.rst

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
adriangb and others added 2 commits April 23, 2023 13:19
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

I am -1 on this, because it can backfire for a lot of tools that rely on the fact that {types,typing}.GenericAlias (and nothing else) represent generic types.

For example, https://github.com/HypothesisWorks/hypothesis/blob/e33377305225df0a4ee50a1bd55ed801df6d1ea2/hypothesis-python/src/hypothesis/strategies/_internal/types.py#L298 (and in many other places).

I think that this needs more visibility and discussion before getting accepted.

@adriangb
Copy link
Contributor Author

Would those things backfire any more now than they currently do? Clearly, this PR doesn't make everything automatically work for every library. Anything that is currently doing introspection and trying to detect typing.GenericAlias (which is sort of a private thing, only mentioned once in passing in typing.rst) would need to add some sort of _is_generic_alias_like check or special case the particular library (Pydantic already has a Hypothesis plugin). But I don't think that makes things worse than the current status quo.

The best alternative to the current implementation/proposal I can think of is to make this stuff more "public" by providing a Protocol, exporting a public is_generic_alias_like function, or making GenericAlias subclassable. Then things like Hypothesis would "just work", which is I think what you want. I feel that the barrier for that is much higher since GenericAlias is this semi-private mostly undocumented thing.

@JelleZijlstra
Copy link
Member

Let's keep broader discussion about whether this is a good idea to the issue, not this PR.

@Zac-HD
Copy link
Contributor

Zac-HD commented Apr 25, 2023

I am -1 on this, because it can backfire for a lot of tools that rely on the fact that {types,typing}.GenericAlias (and nothing else) represent generic types.

For example, HypothesisWorks/hypothesis@e333773/hypothesis-python/src/hypothesis/strategies/_internal/types.py#L298 (and in many other places).

Hypothesis only uses GenericAlias in our has_type_arguments(), is_generic_type(), and "is from the typing module" predicates. , so I think this is very unlikely to be a problem for us. So long as there's some way to inspect at runtime, I can maintain the various code paths just fine 🙂

More generally, I never want Hypothesis doing something bizzare internally to be a blocker for upstream improvements. I'm perfectly comfortable with typing._GenericAlias breaking my code; it's even got the _-prefix to mark that I should expect that!

@adriangb
Copy link
Contributor Author

It seems like improving docs around subclassing types.GenericAlias as well as maybe improving some of the ergonomics of subclassing it is the path forward here, so I'm going to close this PR at least for now. Discussion can continue in the issue.

@adriangb adriangb closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes awaiting review DO-NOT-MERGE stdlib Python modules in the Lib dir topic-typing type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants