search

KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.38k stars 474 forks source link

Consider static type checking #205

Open klardotsh opened 3 years ago

klardotsh commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

We just started releasing it automatically.

sofubi commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

See #226 for my current draft PR.

Jawfish commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

klardotsh commented 2 years ago

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 commented 2 years ago

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.