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

Consider static type checking #205

Open
klardotsh opened this issue Jun 16, 2021 · 12 comments
Open

Consider static type checking #205

klardotsh opened this issue Jun 16, 2021 · 12 comments

Comments

@klardotsh
Copy link
Member

Probably something like https://github.com/Microsoft/pyright (or maybe https://pyre-check.org) would help us sure up a lot of the insanity that happens at our interface boundaries.

I don't think Micro/Circuit Python support PEP-526, so we'd be leaning on PEP-484 and whatever subset of pyright's features can fit PEP-484 syntax, but that's alright. Alternatively, we could check to see if there's a Python 3.10 to 3.5 transpiler out there that would strip, say, type aliases or rewrite pattern matches, at compile time (given that my day job is TypeScript, I'm innately familiar with this concept, but I don't know if it's made its way into Python-land yet. It could be an interesting project to write if it doesn't exist, though...)

Another thing worth noting is that we'd need to ensure that https://github.com/python/typeshed (or a similar collection we and/or Adafruit maintains - this might be something to chat with @tannewt about) has type stubs for CircuitPython's stdlib, which would be a fairly non-trivial undertaking. We only use a small subset of CircuitPython's API, though, so doing a partial type stub writeup probably wouldn't be nightmarish.

@klardotsh
Copy link
Member Author

Other rationale: we basically don't have automated tests (very early on I started down the road of trying to do functional testing, but mocking out the MicroPython APIs proved tricky, so eventually they were removed from the tree), so a type system would at least ensure all the proverbial lego bricks were the right shape, even if not validating their behaviors precisely.

@tannewt
Copy link
Contributor

tannewt commented Jun 23, 2021

The typeshed equivalent for CircuitPython is the circuitpython-stubs package: https://pypi.org/project/circuitpython-stubs/#history

We just started releasing it automatically.

@klardotsh klardotsh pinned this issue Aug 10, 2021
@sofubi
Copy link

sofubi commented Aug 11, 2021

@klardotsh @kdb424 as we've discussed over in Matrix I'm going to try to take this on.

I do have a few questions that I feel are worth keeping in here just so we can track them.

  1. Is there a target Python version I should be working for?
  2. As stated above by @klardotsh "I don't think Micro/Circuit Python support PEP-526, so we'd be leaning on PEP-484 and whatever subset of pyright's features can fit PEP-484 syntax" I'll work on getting the codebase typed and then use the circuitpython-stubs package when needed.
  3. I believe hid.py was a file that maybe I should avoid- are there any other places worth avoiding?
  4. Any really great places to start?

I'll stay in touch via Matrix but if anything important comes up pertaining to this thread I'll be sure to move conversation here.

Thanks!

@kdb424
Copy link
Contributor

kdb424 commented Aug 11, 2021

Circuitpython mostly targets 3.4, but has some extras like fstrings from 3.6 thanks to klardotsh. HID is somewhere we should avoid as it will eventually need replaced by upstreams hid anyways, but otherwise, sounds reasonable to me.

@klardotsh
Copy link
Member Author

Probably the best places to start are the most recent files: anything pertaining to modules (kmk/modules/__init__.py and all sibling files) should be fairly straightforward to type out, as should kmk/kmktime.py, kmk/types.py and kmk/consts.py. From there..... you'd have to dive into the beast that is kmk/kmk_keyboard.py, which will probably cause the most mayhem, as there's still some tangly bits in there, and because it calls into all those other files (so you'll get validation over whether the type signatures are actually representative of current behavior)

@sofubi
Copy link

sofubi commented Aug 16, 2021

See #226 for my current draft PR.

@Jawfish
Copy link
Contributor

Jawfish commented Jul 1, 2022

I opened an issue (#497) before seeing this one. I was under the impression CircuitPython didn't support PEP-526's type annotations, but I accidentally instinctively included some type hinting in some of my code and didn't run into any issues. They're using standard type hinting on this Adafruit page about typing in CircuitPython. They also import typing to use its helper classes during development, but ignore the ImportError at runtime.
Am I missing something? If not, I'd like to go ahead and start adding type hinting to KMK as mentioned in the issue I opened, using the work @sofubi started as reference.

@klardotsh
Copy link
Member Author

I'm fine with bumping CircuitPy minimum version to whatever added support for that PEP if we can figure out when it happened (or just say "current latest is new minimum", if that's not too much community turbulence). This would be dope - I've largely moved off of Python in everything in my life in favor of stronger-typed languages and have been pushing type systems' use at work (Ruby) heavily; I definitely don't need sold on the benefits to approve someone adopting that branch :)

@Jawfish
Copy link
Contributor

Jawfish commented Jul 1, 2022

It looks like support for PEP-526 type annotation was merged in this PR, according to this issue, which was included starting from version 7.0. 7.0's full release occurred less than one month after the previous work on adding type annotations - that's unfortunate. I would not object to continually targeting the latest stable version, but I like living on the edge.

Regardless, I will begin work on adding type hints.

@tannewt
Copy link
Contributor

tannewt commented Jul 1, 2022

We're using typing in our CircuitPython libraries now so it should be fine here too.

@klardotsh
Copy link
Member Author

Thanks @tannewt, helpful context that KMK won't be the odd one out for taking PEP-526 sigs into mainline!

And thanks @Jawfish for diving back into this! I'll be excited to get some review eyes on this whenever you have something to share.

Tangentially, on KMK side: Much like the Rust community tracks Minimum Supported Rust Version (MSRV), we should probably track this in the main readme of KMK - a few times now we've done major bumps to take on new CPy features (the last big one was F-strings way back when, so it's at least infrequent), and we should be front-and-center about those deps. Right now that info is buried in the Getting Started sub-doc, and is pretty loosey-goosey in its wording ("It is best to use the last stable version (>7.0).")

@Jawfish
Copy link
Contributor

Jawfish commented Aug 18, 2022

I've found myself being unable to give this the time and effort that I would like to lately. I intend to chip away at it as I find time to, but I figured I would mention here that it's slow going in case any other interested parties come across this conversation and would be otherwise dissuaded.

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 a pull request may close this issue.

5 participants