Skip to content
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

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

kylebarron
Copy link
Contributor

@kylebarron kylebarron commented Feb 24, 2022

Changes

  • Make this _API_FUNCTIONS class inherit from Generic 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.

from h3.api import basic_int
from typing import List, Set

out: Set[int] = basic_int.k_ring(1, 2)
# Passes

out2: Set[str] = basic_int.k_ring(1, 2)
# error: Incompatible types in assignment (expression has type "Set[int]", variable has type "Set[str]")

out3: List[int] = basic_int.k_ring(1, 2)
# error: Incompatible types in assignment (expression has type "Set[int]", variable has type "List[int]")

out4 = basic_int.k_ring('a', 2)
# error: Argument 1 to "k_ring" of "_API_FUNCTIONS" has incompatible type "str"; expected "int"

Todo

Ref #194 (comment)

@kylebarron kylebarron changed the title WIP: Types for class-based api_functions Add type hints to API_FUNCTIONS class Apr 9, 2022
@kylebarron kylebarron changed the title Add type hints to API_FUNCTIONS class Add type hints to api functions class Apr 9, 2022
@kylebarron kylebarron marked this pull request as ready for review April 9, 2022 17:19
@@ -12,6 +12,7 @@ jobs:
runs-on: ${{ matrix.os }}

strategy:
fail-fast: false
Copy link
Contributor Author

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

@kylebarron
Copy link
Contributor Author

I still need to add type tests, but this should be good for an initial review

Comment on lines +24 to +28
try:
from numpy.typing import NDArray
ArrayType = NDArray[np.uint64]
except ImportError:
ArrayType = np.ndarray
Copy link
Contributor Author

@kylebarron kylebarron Apr 9, 2022

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.

@kylebarron kylebarron marked this pull request as draft April 18, 2022 13:54
@kylebarron
Copy link
Contributor Author

Converting to a draft just to avoid building wheels on CI for now

@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d1a5a3b) 100.00% compared to head (22d2b3d) 99.32%.
Report is 18 commits behind head on master.

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     
Files Coverage Δ
src/h3/api/basic_int/_binding.py 100.00% <100.00%> (ø)
src/h3/api/basic_str/_binding.py 100.00% <100.00%> (ø)
src/h3/api/memview_int/_binding.py 100.00% <ø> (ø)
src/h3/api/_api_template.py 99.47% <93.33%> (-0.53%) ⬇️
src/h3/api/numpy_int/_binding.py 85.71% <71.42%> (-14.29%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kylebarron
Copy link
Contributor Author

I started the test setup. I'll try to flesh out the actual tests later this week.

@kylebarron
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant