-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Support unsubstituted type variables with both a default and a bound or constraints #10789
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
Support unsubstituted type variables with both a default and a bound or constraints #10789
Conversation
CodSpeed Performance ReportMerging #10789 will not alter performanceComparing Summary
|
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
|
please review |
82ccf3b to
e519871
Compare
7961f35 to
183b6fa
Compare
70991c2 to
620d58a
Compare
|
Thanks for your contribution! We'll review this soon - working on pushing v2.10 out now, but I'm anticipating we can get this into v2.11 :) |
9840763 to
58bea02
Compare
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.
Left some initial thoughts... I think maybe we could support an identical bound and default. From the original issue, this would require some addition like:
if bound is not None and default is not None and bound != default:
raise RuntimeError(...)Happy to get @Viicos' thoughts here too, he's got some great insight on detailed types like this.
|
I think it's safe to support all combinations with bounds, constraints and defaults. In any case, we prioritize the default type over T1 = TypeVar('T1', bound=int, default=SubInt) --> generates a schema for `SubInt`
T2 = TypeVar('T2', int, str, default=int) --> generates a schema for `int`Having a different bound and default shouldn't be an issue. The only requirement -- from a static type checking perspective -- is that This should work: def _unsubstituted_typevar_schema(self, typevar: typing.TypeVar) -> core_schema.CoreSchema:
try:
has_default = typevar.has_default()
except AttributeError:
# Happens if using `typing.TypeVar` on Python < 3.13
pass
else:
if has_default:
return self.generate_schema(typevar.__default__) # pyright: ignore[reportAttributeAccessIssue]
if typevar.__constraints__:
return self._union_schema(typing.Union[typevar.__constraints__])
if typevar.__bound__:
schema = self.generate_schema(typevar.__bound__)
schema['serialization'] = core_schema.wrap_serializer_function_ser_schema(
lambda x, h: h(x), schema=core_schema.any_schema(),
)
return schema
return core_schema.any_schema() |
|
Totally agree that it's safe. This code only works for unsubstituted models, so we simply prioritize the default type (expected behavior for me). I will take Viicos suggestion |
58bea02 to
d03ad87
Compare
4d46a2b to
cd4db43
Compare
It doesn't, only static type checkers enforce it :) |
1a4c49e to
d6f8780
Compare
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.
This LGTM otherwise, just one minor suggestion :)
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
db08e3e to
a3687bf
Compare
Co-authored-by: Sydney Runkle <54324534+sydney-runkle@users.noreply.github.com>
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.
Thanks!
Change Summary
Add support for Bound and Default together in TypeVar:
Related issue number
fixes #9418
Checklist
Selected Reviewer: @sydney-runkle