Add type hints#70
Draft
Daverball wants to merge 1 commit into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is based on the stubs I had already written, but I improved them significantly based on what type checking the tests revealed about the ergonomics.
Some of the type hints are a bit convoluted and difficult to understand, but I don't think we can simplify much without essentially making the type hints useless.
One minor wart these current hints have, is that using the
dispatchdecorator on a method/classmethod will result in false positives when those methods are invoked on instances of that class. While this is technically solvable to a degree, it will introduce even more protocols and complex overloads and since you're generally supposed to usedispatch_methodon methods anyways, to get the proper benefits from the caching lookups, I think it's a reasonable trade-off, even though the test suite verifies that usingdispatchworks on methods, so I've had to skip type checking for that code-path.For now I've added tox environments for mypy and pyright. I also very quickly looked at ty and zuban. Neither produced a ton of errors, but it didn't seem worth adding two more sets of pragmas for the few errors they did spit out, especially considering they're both still fairly new and ty doesn't even properly support
ParamSpecyet. We can revisit adding support for these more modern rust-based type checkers later.I would like to add a few dedicated type tests (even though the existing tests already do a good job of exercising the API, it would still be good to put a few
assert_typein there, to verify that we're getting the types we think we're getting) and I still need to do a self-review, since usingmerge-pyias a starting point almost certainly put some annotations in stupid places, that I missed, so I'll put this in draft for now, but feel free to already take a look at it @henri-hulski and test against your code. I've tested the updated type hints against my code and it helped get rid of a couple oftype: ignorecomments that have now become redundant, other than that things stayed green, so overall an improvement.