Skip to content

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 Standard Library Python modules in the Lib/ directory 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 Standard Library Python modules in the Lib/ directory topic-typing type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants