Skip to content
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

clarify string types in pyi #118

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

tlambert03
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

tlambert03 commented Jun 18, 2024

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
Copy link
Member

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?

  • Have DeviceLabel be the NewType (that decays to str)
  • No separate ValidDeviceLabel
  • Methods returning a known device label are annotated to return DeviceLabel
  • Methods taking an (existing or new) device label are annotated to take DeviceLabel | str

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
Copy link
Contributor Author

tlambert03 commented Jun 19, 2024

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

brilliant :) will made that change

Copy link
Member

@marktsuchida marktsuchida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

src/pymmcore/__init__.pyi Outdated Show resolved Hide resolved
@marktsuchida marktsuchida merged commit 192bd58 into micro-manager:main Jun 19, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants