-
Notifications
You must be signed in to change notification settings - Fork 4
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
Include generics in discriminated union schemas #157
Conversation
6b0c40c
to
06e7542
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.
From a skim this looks fine to me
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 nit and a possible edge case
src/scanspec/core.py
Outdated
def _compatible_types(left: TypeVar, right: TypeVar) -> bool: | ||
return ( | ||
left.__bound__ == right.__bound__ | ||
and left.__constraints__ == right.__constraints__ | ||
and left.__covariant__ == right.__covariant__ | ||
and left.__contravariant__ == right.__contravariant__ | ||
) |
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.
Could left.__ be a subset of right.__?
class Super:
class Sub(Super):
@discriminated...
class Parent(Generic[Super]):
class Child(Parent[Sub], Generic[Sub])
e.g. for class ReadableMovable(Movable)?
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.
You can't pass concrete types to Generic
as in this example but yes, this is definitely restricting the supported cases beyond what is possible in theory. It turns out to be difficult to determine what should be passed without re-implementing an entire type-checker. There are a couple of examples in #156 and a few in a previous version of this commit where I tried to support a wider range of options but until we have a use case for them I'd prefer a well defined restriction instead of an array of unsupported edge-cases.
06e7542
to
08e4d1a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
- Coverage 95.74% 95.70% -0.05%
==========================================
Files 9 9
Lines 940 977 +37
==========================================
+ Hits 900 935 +35
- Misses 40 42 +2 ☔ View full report in Codecov by Sentry. |
Delete uncovered lines as discussed |
Should consider adding a codecov config like ophyd-async's for consistency |
Without allowing arbitrary types, instances of Spec such as `Line[Movable]` cannot be validated as Movable cannot be serialised.
216f797
to
c8c4494
Compare
Rebased to pick up codecov changes |
Fixes #156
Instructions to reviewer on how to test:
Child
as aBase
:Checks for reviewer