-
Notifications
You must be signed in to change notification settings - Fork 183
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
Add type hints to openslide
package
#260
Conversation
DCO signed off ✔️All commits have been signed off. You have certified to the terms of the Developer Certificate of Origin, version 1.1. In particular, you certify that this contribution has not been developed using information obtained under a non-disclosure agreement or other license terms that forbid you from contributing it under the GNU Lesser General Public License, version 2.1. |
Thanks for the PR! #243 is currently waiting for me to review it. I have a few other things to finish first, but it's on my list. mypy for ≥ 3.8 sounds good; that's what we've been using elsewhere. Ideally we'd type lowlevel too, but I'm not sure yet how much of a pain that will be. I know @jaharkes was also working on this. |
c0d19c3
to
0b6e742
Compare
Just fixed the tests, and remembered I also updated some pieces which previously output tuples to lists as their size was dynamic. I imagine this could be a problem and a technically a breaking change for those expecting tuples. |
Some initial comments:
Also cc @jaharkes for review. |
Also, if we use |
Not only that, but we could also use It looks like PEP 649 has opted for a third approach, neither classic annotations nor string-typed annotations, starting in Python 3.13. As I read the backward-compatibility section, annotated objects under PEP 649 will still have slightly different semantics if |
sure thing, apologies I assumed that was precommit
happy to use future
I think there was but may have just been a different battle with the type checker. I'll take a proper look at making the changes soon. Cheers |
043d2c8
to
3b621bd
Compare
hey @bgilbert, suggestions applied and pushed.
|
6f4b6e0
to
01bf0b1
Compare
I've pushed #277 to require |
Rebased again to pick up more CI changes. |
Signed-off-by: Sam Maxwell <[email protected]> Signed-off-by: Benjamin Gilbert <[email protected]>
Signed-off-by: Sam Maxwell <[email protected]>
Signed-off-by: Benjamin Gilbert <[email protected]>
Signed-off-by: Benjamin Gilbert <[email protected]>
Rebased onto #281, removed some |
AbstractSlide subclasses will always return their specific type from this method. Ensure the type system knows this. Signed-off-by: Benjamin Gilbert <[email protected]>
bytes will not work in at least one case (the OpenSlide initializer), are not documented to work, and the type annotations are currently inconsistent about accepting it. For now, forbid it everywhere for consistency. Signed-off-by: Benjamin Gilbert <[email protected]>
open_slide() can't return any possible AbstractSlide subclass, only the two that it's documented to return. Make that explicit in the type hint. Signed-off-by: Benjamin Gilbert <[email protected]>
Avoid overcommitting to internal implementation details in the types being returned. It's sufficient to say that the property and associated image maps implement Mapping. Signed-off-by: Benjamin Gilbert <[email protected]>
We don't want to encourage library users to duck-type a replacement for OpenSlideCache, since any replacement would inherently need to make assumptions about our implementation details. Signed-off-by: Benjamin Gilbert <[email protected]>
When an operation is performed on a closed ImageSlide, we've historically raised an AttributeError upon dereferencing None. Add proper checks so the type system understands what we're doing, raising ValueError as lowlevel does. This is not an API change because it only affects invalid caller behavior. Signed-off-by: Benjamin Gilbert <[email protected]>
We don't need to avoid generator expressions to keep the type checker happy; we can just assert that the resulting tuples have the correct length. This lets us avoid carrying redundant unrolled code. Signed-off-by: Benjamin Gilbert <[email protected]>
level_dimensions is always a tuple of 2-tuples. Prove this to the type checker. Signed-off-by: Benjamin Gilbert <[email protected]>
Keep the type checker happy by asserting the correct tuple lengths rather than unrolling generator expressions. Signed-off-by: Benjamin Gilbert <[email protected]>
__init__() methods don't need a return type hint unless they take no arguments. Signed-off-by: Benjamin Gilbert <[email protected]>
ImageSlide is hardcoded to a single level, and the level_count property and get_best_level_for_downsample() return types already reflect this. Reduce the level_dimensions() and level_downsamples() return types to 1-tuples. In the latter case we can't reduce to a Literal because literal floats are disallowed. Signed-off-by: Benjamin Gilbert <[email protected]>
Okay, I think this is ready to go. On further reflection, it seems useful to preserve the context from the cleanup commits, so I'll merge normally rather than squashing. Thank you very much for doing this! |
Hey folks,
An attempt to address #157
I've typed the main interface and the deepzoom file, with ignoring type hints for the harder-to-type lowlevel file.
I think I could get there for lowlevel as well but would this suffice in the meantime?
Would have a few conflicts with #243, is that still active?
I've strict typed against 3.8 as per the mypy settings in pyproject.toml. Opted for mypy as that's the tool I'm familiar wth