-
Notifications
You must be signed in to change notification settings - Fork 132
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 type hints to api functions class #216
Conversation
@@ -12,6 +12,7 @@ jobs: | |||
runs-on: ${{ matrix.os }} | |||
|
|||
strategy: | |||
fail-fast: false |
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 should be reverted before merging
I still need to add type tests, but this should be good for an initial review |
try: | ||
from numpy.typing import NDArray | ||
ArrayType = NDArray[np.uint64] | ||
except ImportError: | ||
ArrayType = np.ndarray |
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.
The numpy.typing module is new in v1.20: https://numpy.org/devdocs/reference/typing.html
We might want to check in the tests that this works with numpy both before and after v1.20.
Converting to a draft just to avoid building wheels on CI for now |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #216 +/- ##
===========================================
- Coverage 100.00% 99.32% -0.68%
===========================================
Files 17 17
Lines 425 447 +22
===========================================
+ Hits 425 444 +19
- Misses 0 3 +3 ☔ View full report in Codecov by Sentry. |
I started the test setup. I'll try to flesh out the actual tests later this week. |
It's pretty tedious to write all the type tests, so I still haven't finished them, but you can see the last three commits: https://github.com/uber/h3-py/pull/216/files/58f5e4f9dcc188a77eb0b33e4cc03dec82777d0a..22d2b3d7ecd25a4283be13fd71fec5b73781fad3 for how the type tests are written. |
Closing since it has been a while and the repo has diverged. Happy to revisit in the future. |
Changes
_API_FUNCTIONS
class inherit fromGeneric
to support its use as a generic class. This solves the typing problem without needing to keep track of type hints defined separately from code and with no duplication (see closed PR Add type stubs #194).Benefits:
Backwards compatible with Python 2.7
Uses type hints inside comments for backwards compatibility at runtime. If/when we deprecate support for Python 2.7 we can move the type hints into function parameters. All existing tests are passing.
Typing without separate stub files
Defines
_API_FUNCTIONS
as a generic class, which can be parametrized with different scalar types. This allows mypy to understand function typing.Todo
TODO
comments in the code)api_functions
for better tooling support #213 (comment)h3.unstable.vect
.Ref #194 (comment)