micro-manager / pymmcore

Python bindings for MMCore, Micro-Manager's device control layer
https://pypi.org/project/pymmcore/
GNU Lesser General Public License v2.1
33 stars 8 forks source link

clarify string types in pyi #118

Closed tlambert03 closed 3 months ago

tlambert03 commented 3 months ago

There are lots of strings used throughout the CMMCore API, but using the wrong type of string in the wrong place can raise exceptions. This PR clarifies those string names using type hints. At the moment, I'm actually using NewType, but may relax that before this merges. (NewType actually triggers a static typing error if you use anything other than the actual newtype, which is rather strict, but ensures safety). A safer intermediate would be to use NewType for string types in return values, but use TypeAlias strings for parameters used as inputs

marktsuchida commented 3 months ago

It amazes me what can be done by type annotations alone!

I think I like the "intermediate" approach. Would I be right that this would trigger type check errors if you pass the wrong kind of string that was previously returned by a CMMCore method, but would not be an error if you just use a plain str?

tlambert03 commented 3 months ago

yeah, the intermediate approach (which I now changed this to) will basically never result in static type errors, unless the user (such as pymmcore-plus) opted into stricter checking by annotating something like a variable or a function parameter as one of the NewType variants. I still think it's visually useful to have the type aliases, to disambiguate all the various str types.

In other words, as this PR currently stands, the following will still be just fine from a typing perspective.

core.loadDevice('WhateverYouWant')

but, if you want to, you could do this to indicate that a parameter is only valid if it was returned from the core:

import pymmcore

core = pymmcore.CMMCore()

def strict_init(device_label: pymmcore.ValidDeviceLabel) -> None:
    core.initializeDevice(device_label)

# Argument 1 to "strict_init" has incompatible type "str"; expected "ValidDeviceLabel"
strict_init("Camera")

# OK
strict_init(core.getLoadedDevices()[0])
tlambert03 commented 3 months ago

the only downside here, which you should be aware of, is that to a newcomer, these type aliases may at first appear to be special objects (since type aliases show the alias name, rather than what they resolve to)

Screenshot 2024-06-18 at 1 44 03 PM

of course, in an IDE you can click on those symbols and in two clicks get to this:

Screenshot 2024-06-18 at 1 45 26 PM

so I think it's preferable, but your call

marktsuchida commented 3 months ago

I see, but I think that is not too big a downside, especially if it's just one more click. Much more helpful to have the string category clarified, as you say.

But (now that I've seen how this looks) how would this compare to doing the following?

The main advantage being simplicity (validity can change over time so cannot truly be enforced by typing anyway). But it also happens to solve the beginner problem of not being clear that a str can be used.

tlambert03 commented 3 months ago

Methods taking an (existing or new) device label are annotated to take DeviceLabel | str

brilliant :) ~will~ made that change