Typing

[sammaxwellxyz]

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

[openslide-bot]

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.

[bgilbert]

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.

[sammaxwellxyz]

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.

[sammaxwellxyz]

I've changed the zip pattern in a few places to directly construct the 2-element tuples so that the type checker knows it's a 2-element tuple.

[bgilbert]

Some initial comments:

• Please remove the quoting changes. They make the diff hard to read, and (as is hopefully obvious) OpenSlide Python mostly uses single quotes for strings. Likewise, I think the .gitignore changes are unrelated.
• In other Python-based OpenSlide code (e.g. openslide-bin) we use from __future__ import annotations and enforce its presence via CI. That would avoid the need for moving _OpenSlideMap etc. within the source file. I'm interested in opinions about whether that's the right approach here. I know there have been multiple abandoned attempts to make Python use that import by default, and adding it here would affect the OpenSlide Python API, so it's possible we should avoid it.
• If we don't add that import, please do the moving of _OpenSlideMap etc. in a separate preparatory commit that doesn't change anything else. That makes the later commit easier to read without move-aware diffing.
• I'd rather not convert tuple return values to lists, for the reason you mentioned. Is there a reason the tuple[T, ...] syntax won't work here?

Also cc @jaharkes for review.

[bgilbert]

Also, if we use from __future__ import annotations, we can continue to support Python 3.8 while avoiding the deprecated aliases.

[bgilbert]

Not only that, but we could also use a | b instead of Union and a | None instead of Optional.

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 from __future__ import annotations is active when they're defined. Even so, I'm leaning toward using the import. It would substantially improve the clarity of type annotations within the OpenSlide Python codebase.

[sammaxwellxyz]

Please remove the quoting changes. They make the diff hard to read, and (as is hopefully obvious) OpenSlide Python mostly uses single quotes for strings. Likewise, I think the .gitignore changes are unrelated.

sure thing, apologies I assumed that was precommit

In other Python-based OpenSlide code (e.g. openslide-bin) we use from future import annotations and enforce its presence via CI. That would avoid the need for moving _OpenSlideMap etc. within the source file. I'm interested in opinions about whether that's the right approach here. I know there have been multiple abandoned attempts to make Python use that import by default, and adding it here would affect the OpenSlide Python API, so it's possible we should avoid it.

happy to use future

I'd rather not convert tuple return values to lists, for the reason you mentioned. Is there a reason the tuple[T, ...] syntax won't work here?

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

[sammaxwellxyz]

crop expects a 4-tuple rather than an n-tuple

[sammaxwellxyz]

the changes over the next few lines are to allow for the returned tuples to be 2-tuple rather than n-tuples where appropriate to fit with other calls e.g read_region

[sammaxwellxyz]

hey @bgilbert, suggestions applied and pushed.

• quotes back to the way they were
• adopted future annotations, no stringified return type for Self and use of union operator
• separate commit for the _OpenslideMap, not necessary with future annotation but figured it won't hurt
• dropped using lists for the n-tuples, but have left in some changes to use 2-tuples

[bgilbert]

I've pushed #277 to require from __future__ import annotations everywhere, rebased this PR to fix CI, and dropped the move of the _OpenSlideMap classes since it's not needed anymore. Will circle back to review this soon.

[bgilbert]

Rebased again to pick up more CI changes.