-
Notifications
You must be signed in to change notification settings - Fork 6
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
Enable professional-grade mypy #129
Conversation
46dca74
to
9173823
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
=======================================
+ Coverage 85.9% 87.0% +1.0%
=======================================
Files 227 228 +1
Lines 7583 8155 +572
=======================================
+ Hits 6514 7095 +581
+ Misses 1069 1060 -9
|
If we adopt this PR, we should also present a mypy badge in our readme :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, what a feat! Thanks a lot, this is very cool.
I couldnt find what you mentioned in point 5. It sounds like that should be possible, but I've seen ParamSpec for the first time in this PR and couldnt tell you exactly what to do..
I also left some comments inline.
Okay, the update is here :) I still need to do some cleanup, I want to look into your suggestion of recursive hints (and maybe employ some |
e44a0c9
to
72f9bb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took longer than expected again, but here it is: calling out every single thing I had a question about or an issue with! I'll push one or two more clean-up commits with things I noticed while going through all files, but after that, I'm happy with this PR :)
* Complete type hints, even for tests * Bump mypy to latest release version
72f9bb1
to
3fb15b9
Compare
Finally, here it is!
After reading this blog post about "professional-grade" mypy, I began adopting the professional-grade config for ixmp4. This PR is the result of the process.
It type hints everything, though some type hints might still need relaxation or tightening. Even our test suite is type-hinted here. This allows us to trust all variables we're dealing with have the correct type. In particular, refactoring can be done with improved confidence as mypy would catch a wide variety of bugs if the refactoring doesn't fit the existing code. Having these type hints also helps with readability, I think, as it is one kind of documentation in itself. I found this process especially insightful for understanding how the filters work :)
Of course, this PR also bumps mypy to its latest version.
That being said, there are a few points where I think we can still improve the PR, or add improvements through a future PR:
create()
functions in the DB layer, I often kept the*args
and**kwargs
since the facade layer often uses positional arguments, while the rest/api layer uses keywords. This is different fromtabulate()
andlist()
, which are generally only called viaenumerate()
and with kwargs, so this would be a place to streamline the signatures.data/abstract/meta
, I needed to type hint the_type_map
, for which I needed to use the built-intype
. This led to confusion for mypy sincemeta
already had an attribute calledtype
. I have renamed the attribute todtype
to enable the type hint.kwargs
, I have learned that the best way to type hint them is by creating aTypedDict
subclass, which explicitly spells out all possible kwargs and their types. I have not done so for some/base/
functions, which mainly act as templates or pass on all*args, **kwargs
that they receive. Whenargs
orkwargs
were unused, I sometimes removed them from the signature and type-hinted them asAny
at other times, depending on how I understood the function's purpose. So there are currently two approached visible, which come down to different understandings of type hints, I suppose:On the one hand, one could try to spell out everything explicitly how it is currently used. This improves the documentation aspect and enables mypy to catch more bugs before they reach production. On the other hand, this would block newly introduced kwargs until they are added to these kwargs-classes, which increases maintenance burden. Alternatively, one could convey the purpose of the function through the type hints, being more general. The pros and cons are essentially the inverse of the above.
For code that I didn't write, it's harder to correctly guess the purpose of functions, so I have generally opted more for the first approach. However, I'm open to discussion here.
EnumerateKwargs
could often inherit fromCreateKwargs
at the moment. For the filter classes, spelling out everything likename__like
is also lengthy, we could e.g. think of using mixins there./base/
functions that pass on their arguments, I usedAny
because I couldn't get the alternative to work: in theory, this sounds like a job forParamSpec
: "They are used to forward the parameter types of one callable to another callable". However, I couldn't figure out exactly how to do that. On that note, if you know a good tutorial onParamSpec
, I'd be happy to read it as this almost seems like magic to me right now.Lastly, I applied some
type: ignore
markers where I couldn't quite understand why mypy was complaining or where I intend to refactor the code very soon, anyway. In principle, though, almost none of that should be necessary, strictly-speaking. "Code I want to change soon" mainly refers to the DB model for parameters, equations, and variables, where I expect a more performant model can make do without the wholecolumn
thing. And I would maybe also droptable
entirely since this is used nowhere in the tutorials. This is less certain, however, which is why I did type hinttable
, still.Note
As so nicely displayed by codecov, we don't have 100% test coverage yet (that's another juicy small project goal...). So when reviewing this, please use the branch like
main
might be used to ensure that things we don't test did not get broken. In particular, this probably means running some CLI commands from this branch.