h3-py icon indicating copy to clipboard operation
h3-py copied to clipboard

Add type hints to api functions class

Open kylebarron opened this issue 3 years ago • 5 comments

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 #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

  • [ ] Check some of the input/returned types (see a couple TODO comments in the code)
    • [ ] Discussion about whether the numpy int api should return numpy int objects: https://github.com/uber/h3-py/pull/213#discussion_r837198430
  • [ ] Add type tests. See https://github.com/uber/h3-py/pull/194/files#diff-c02f137b8e703d2c3920f300d2929be20f20a5fed41cb453cf4c29221e3d7253 for reference
  • [x] Add type hints to h3.unstable.vect.

Ref https://github.com/uber/h3-py/pull/194#issuecomment-1043368400

kylebarron avatar Feb 24 '22 22:02 kylebarron

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

kylebarron avatar Apr 09 '22 17:04 kylebarron

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

kylebarron avatar Apr 18 '22 13:04 kylebarron

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%) :arrow_down:
src/h3/api/numpy_int/_binding.py 85.71% <71.42%> (-14.29%) :arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 18 '22 14:04 codecov[bot]

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

kylebarron avatar Apr 18 '22 15:04 kylebarron

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.

kylebarron avatar Apr 21 '22 21:04 kylebarron