Put pyright in strict mode #143
Conversation
callumforrester
force-pushed
the
pyright-strict
branch
from
509c056 to
da86e13
Compare
|
#142 fixes schema |
| from pydantic import BaseModel, ConfigDict, Field, GetCoreSchemaHandler | ||
| from pydantic.dataclasses import is_pydantic_dataclass, rebuild_dataclass | ||
| from pydantic_core import CoreSchema | ||
| from pydantic_core.core_schema import tagged_union_schema | ||
|
|
||
| __all__ = [ | ||
| "if_instance_do", | ||
| "Axis", |
There was a problem hiding this comment.
This was in __all__ so the docs didn't complain about a missing ref, please can you check the docs still look ok after this?
There was a problem hiding this comment.
Since I agree with #143 (comment), I'm putting this back, however I was planning to manually check the docs and the plotting once you were happy with everything else, so will leave this thread open as a placeholder for that.
| @@ -32,6 +33,8 @@ | |||
| "find_regions", | |||
| ] | |||
|
|
|||
| NpMask = npt.NDArray[np.bool] | |||
There was a problem hiding this comment.
BTW I recently discovered you can specify shape too, although most of numpy returns shape=Any so it's of limited use at the mo:
ScalarType_co = TypeVar("ScalarType_co", bound=np.generic, covariant=True)
OneDArray = np.ndarray[tuple[int], np.dtype[ScalarType_co]]
TwoDArray = np.ndarray[tuple[int, int], np.dtype[ScalarType_co]]
NPMask = OneDArray[np.bool]There was a problem hiding this comment.
Interesting... worth a separate issue to specify shape where possible?
src/scanspec/specs.py
Outdated
| #: A type variable for an `axis_` that can be specified for a scan | ||
| Axis = TypeVar("Axis", covariant=True) |
There was a problem hiding this comment.
Why re-implement rather than import?
There was a problem hiding this comment.
My mistake, I thought it was best practice, but having researched a bit more I agree that importing TypeVars is the way to go. Case in point: When I removed these it broke because Axis in core.py wasn't covariant. I have now fixed and imported both variables.
| def __mul__(self, other: Spec[Axis]) -> Product[Axis]: ... | ||
|
|
||
| @overload | ||
| def __mul__(self, other: Spec[OtherAxis]) -> Product[Axis | OtherAxis]: ... |
There was a problem hiding this comment.
It's variance like this that makes me think duration should be special, otherwise we end up with lots of Spec[Motor | str] and isinstance checks.
Alternatively we might decide that we always need Spec[str] with a lookup, as the case for Spec[Motor] in ophyd_async is looking tenuous. One for the scanspec v1 ticket (I realised we never made scanspec 1.0 so might as well use that number).
There was a problem hiding this comment.
A few thoughts here:
Spec[Motor]would be very nice for blueapi, if we can get it to work with pydantic, evenSpec[Motor, str].- If we make time special there might be a way to incorporate it into a
Spec[Motor]without making it aSpec[Motor, str](or really aSpec[Motor, Time], which would be preferable) - However we should ask ourselves if the variance is actually a bad thing, the extra verbosity here is just capturing a special case at "compile" time rather than runtime, the special case always existed and was always special, strict mode simply forces us to acknowledge it. In other words it is possible that a bunch of
isinstancechecks really might be the right thing to do here, or it's possible that we can box the specialness into one place by treating time as special somehow. I am not sure, but I don't think we should be prescriptive.
callumforrester
force-pushed
the
pyright-strict
branch
from
19c4acc to
e065997
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
- Coverage 95.89% 95.74% -0.15%
==========================================
Files 9 9
Lines 925 940 +15
==========================================
+ Hits 887 900 +13
- Misses 38 40 +2 ☔ View full report in Codecov by Sentry. |
Fixes #144