Fixes basic typing issues in rasterize.py - #1247
Conversation
There was a problem hiding this comment.
Not sure why my review got requested here but random comments:
- Nothing wrong with also being pyright compliant, but just mypy is good enough for now. As discussed on the template, there's is a likely future where scverse will move to pyright, pyrefly, or ty. We'll see.
- Instead of having lots of asserts, I'd encourage you to raise informative errors. It's more "pythonic".
- I'm all for more dataclasses/namedtuples with descriptive names. Well done!
In the future, we can simplify this by configuring the type checker to be stricter, but one step at a time =)
Yeah! Please feel free to use mypy in the stricted possible mode. But also note that you will have to fight the typechecker from time to time as they're all broken in their own ways.
Just texted you in Zulip, but you saw the notification before the message 😁 |
For these kinds of errors, I'd rather stick to the asserts, because they're not operational errors; They are bugs, and a sign that we don't know what's going on anymore. So the asserts are there for the sake of sanity checking and also for keeping the type checker happy, which enables further sanity checking =D |
Cleans up
rasterize.pyso that it passes basic type checking with pyright.Rationale behind
not isinstance(...)Unfortunately, because python has a crazy (multi-)inheritance model, code like this:
will be accepted by type checkers (when not in strict mode), and will infer
a: <subclass of int and list>, so we can end up accidentally asserting a type that should never be asserted. If we instead assert that the type is not what it isn't supposed to be, the type checker can infer that it must be the correct type. And as a bonus, if ever the type of the asserted value changes, this negative assertion won't hide this from us by forcing the type of the variable to whatever was asserted.In the future, we can simplify this by configuring the type checker to be stricter, but one step at a time =)