-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add pre-commit
to CI and fix all typing and linting
#306
Merged
Conversation
This file contains 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
pre-commit
to CIpre-commit
to CI and fix all typing and linting
Ready for merging! 🥳 |
adehecq
reviewed
Sep 20, 2022
adehecq
approved these changes
Sep 20, 2022
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.
Pffff what a work !! Thank you for doing that.
I must admit it was difficult to distinguish between what was really important change and purely linting changes. I had to scroll quite quickly, but your nice summary was super useful.
This was referenced Sep 21, 2022
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.
Summary
This PR adds
pre-commit
to xDEM's CI, lints all package files with the linters, and manually solves all of thecodespell
errors (~10),flake8
errors (~200) andmypy
errors (~500 errors) raised (+ minor errors from other small modules).Additionally, to avoid
np.ndarray
types being considered asAny
(the default before Python 3.9, and after if theirdtype
is not specified) which prevents us from differentiating them fromRaster
objects in function calls/outputs, this PR adds the NumPy plugin tomypy
(https://numpy.org/devdocs/reference/typing.html#mypy-plugin). This solved several errors but also raised another ~300, now solved.What's new in the
pre-commit
configThe implementation of this PR mirrors that of @erikmannerfelt in GeoUtils, at the exception of the following points:
mypy
now understandsnp.ndarray
types through the plugin supported since NumPy 1.21 called by--config-file=mypy.ini
in.pre-commit-config.yaml
. Themypy.ini
file declares the plugin:[mypy] plugins = numpy.typing.mypy_plugin
.mypy
uses a NumPy version installed locally inpre-commit
, and so is independent from that used in the package, ensuring no issue when testing older Python versions;isort
is now made compatible withblack
to avoid infinite-loop conflicts between the two (that also started happening in GeoUtils recently), by passingargs: ["--profile", "black"]
.mypy
now authorizes implicit optional types in function parameters, for examplex: int = None
is understood automatically asx: int | None = None
, following an old comment of @adehecq and myself. This is done by passing--implicit-optional
tomypy
. Careful: There are exceptions related to class definition and overload.codespell
now ignores several words that we use in xDEM. It was hard to pass several arguments as a list in the args of the.pre-commit-config.yaml
, but I finally managed to make it work from a GitHub issue, the syntax is (need to convert to list of strings):['--ignore-words-list', 'nd,alos', '--ignore-regex', '\bhist\b', '--']
(the'--'
serving to indicate the end of the list of arguments);flake8
is now more flexible on the writing of dicts. This is done by ignoringC408
errors that forbids the writing of dicts other ways as with literals (e.g.,dict(a=b)
is forbidden, but{"a":b}
is fine). This can be a bit annoying when we define dictionaries with keys typically corresponding to function arguments.pre-commit
have been upgraded.What's new in xDEM's type hinting
mypy
typechecking. For this, this PR uses theNDArray[X]
NumPy generic array type which is a convenience wrapper corresponding tonp.ndarray[Any, np.dtype[X]]
. In xDEM, we use almost only floating types, and so this PR defines two new NumPy-based dtypes:NDArrayf
, corresponding toNDArray[np.floating[Any]]
, i.e. an array of any shape with any floating type. AndMArrayf
, corresponding tonp.ma.masked_array[Any, np.dtype[np.floating[Any]]]
. This syntax is only supported by Python >= 3.9, and so is made compatible in a new_typing.py
module:Coreg
classes andspatialstats.py
objects were a bit of a mess for type checking, because they both use dictionaries with many different keys:Coreg._meta
in the first case, and various**kwargs
arguments in the second. To address this, this PR addsTypeDict
objects to describe the components of these dictionaries. For instance, forCoreg
classes:float
had to be changed tonp.floating[Any]
now that NumPy is supported. However, this is not always the case and is still inconsistent for now. At a later stage, we could create types such asFloat = float | np.floating[Any]
to ensure typing is consistent everywhere. Here's an example fornmad
that fails with afloat
return type:@overloading
statements are "saved" 👼 by the new NumPy array plugin. Nowmypy
differentiatesRaster
andNDArrayf
(not considering them both asAny
anymore and ignoring the overloading). However, a lot of issues were triggered still bymypy
which were really hard to resolve. This is because of two things: 1/ the broadest type overload must always be listed last; 2/ all parameters defaults must be fully copied in the overload (error message quite unclear).For example of 1/, this will fail...
...because
RasterType
is considered asAny
! So the first@overload
would also work forNDArrayf
, making the second one irrelevant. To solve this, the second@overload
withNDArrayf
needs to be moved to the first position.I added these points to xDEM's wiki page on Tips to make mypy happy.
Resolves #53
Final note: is all of this really useful?!
I was skeptical about type hinting when we started doing it. My experience from correcting hundreds of type errors in xDEM in the past days has made me more decisive: Although it can be a bit of a pain for some errors because we aren't used to them (the Wiki on this is growing! 💪), type checking will save us a lot of time on possible type-related errors down the road! 😁
Why? Well, if we look at this xDEM PR: we were already type hinting during the past years (and I was respecting the internal type checker of PyCharm while adding stuff). Despite this, there were still ~800 errors that were raised by
mypy
(about ~600 redundants one solved in a couple dozen fixes). From going through the ~200 other individual errors, I would say that about ~50 of those would have created small bugs/issues at some point (illogical passing of objects between functions, too broad user-input, etc..). And, actually, ~10 of those helped correct little errors that existed in our libraries! 🙂Go type hinting! 🦸