Add type stubs #194
Add type stubs #194
Conversation
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 213 213
=========================================
Hits 213 213
Continue to review full report at Codecov.
|
|
Cool, this is great! I'll try to take a look at this sometime next week. |
|
New to |
That's a good question.
|
|
Did you want to land this as-is, or are you still planning on tackling some of the TODOs you listed above? Also, do you any idea on the pros/cons of storing the stubs in https://github.com/python/typeshed instead of within the repo? (If I'm understanding things correctly that that is a possibility.) |
|
Oh cool you can see the commit comments inside this PR!
No I think a couple things should be resolved first.
I learned how to do this using https://github.com/TypedDjango/pytest-mypy-plugins, see: https://github.com/kylebarron/types-affine/blob/master/tests/test_affine.yml. Would be good to add. When I was originally hacking on these types, I never hit the
Yes that is an alternative. I think that would be the place to go if
I think including the types with the package itself is seen as more "official". If a package starts to include types, typeshed's policy is to remove its stubs 6-12 months later. |
|
Note I tried to incorporate the suggestion from python/mypy#6073 (comment), but that didn't work because I think the best option is to copy the text of should change in any file since we can just change the parameterized aliases at the top of the file. We could even write a tiny script for CI to verify that the content below that line is identical in each api file. |
|
@ajfriend It was a little annoying to try and get the test scaffolding set up, since the type tests are only run on python 3.6+. Maybe it would actually be easier to separate tests into two CI steps, change the existing one to At this point it looks like what's left to do is:
|
Separating out tests would be nice, and I think it would be fine to run the tests for a subset of Python versions, or even just a single Python version. Having the a separate CI workflow would also make it easier to spot if a build issue was coming from the typing or normal test suite. |
|
I tried to separate out the tests. Do you know how to fix the Windows tests? I guess you can't use |
There was a problem hiding this comment.
Re: ulonglong vs uint64: I would recommend uint64 when handling H3 indexes since it's better to be explicit that it is a fixed size and not platform dependent.
Re: Windows: This is probably an issue with how PowerShell resolves the *. Perhaps try specifying shell: bash.
| def string_to_h3(h: str) -> int: ... | ||
| def h3_to_string(x: int) -> str: ... |
There was a problem hiding this comment.
Will this accept H3Cell as input?
There was a problem hiding this comment.
Just noting this was typed as int because h3_to_string always takes an int, no matter the API
h3-py/src/h3/api/_api_template.py
Lines 100 to 101 in 7364b21
| def versions() -> Dict[str, str]: ... | ||
| def string_to_h3(h: str) -> int: ... | ||
| def h3_to_string(x: int) -> str: ... | ||
| def num_hexagons(resolution: Resolution) -> int: ... | ||
| def hex_area(resolution: Resolution, unit: AreaUnits = 'km^2') -> float: ... |
There was a problem hiding this comment.
Is it possible to avoid repeating the entire API definition after defining H3Cell each time? (As below in the PR?)
I've done some poking around as I'm keen to having typing available as well. I believe the change to make for Windows is just to surround arguments with quotes, eg. I created a fork and tried that out and it appears to do the job. |
|
Hmm I tried to make that change but I'm now getting |
|
I think the quotes are only required on the tests, not the typing tests. |
|
pfw@7aa011b This was the change I had success with. Just on tests as the quotes blew up on the typing tests which only ran on Ubuntu. |
|
At least using bash allows for the config to be common across Windows and Linux. |
|
Is this PR still being actively worked on? Am curious if the changes in version 4 will disrupt much of the stub work done here. |
|
I looked back at this PR and had a new (
Here's an example. Replace # _api_template.py
from h3 import _cy
from typing import TypeVar, Generic
HexType = TypeVar("HexType")
class _API_FUNCTIONS(Generic[HexType]):
def __init__(
self,
_in_scalar,
_out_scalar,
_in_collection,
_out_unordered,
_out_ordered,
) -> None:
self._in_scalar = _in_scalar
self._out_scalar = _out_scalar
self._in_collection = _in_collection
self._out_unordered = _out_unordered
self._out_ordered = _out_ordered
def geo_to_h3(self, lat: float, lng: float, resolution: int) -> HexType:
"""
Return the cell containing the (lat, lng) point
for a given resolution.
Returns
-------
H3Cell
"""
return self._out_scalar(_cy.geo_to_h3(lat, lng, resolution))Then create an instance of this class in # __init__.py
from . import _cy
from ._api_template import _API_FUNCTIONS
from .basic_int import _id as _basic_int_id
basic_int = _API_FUNCTIONS[int](_out_scalar=_basic_int_id, ...)
basic_str = _API_FUNCTIONS[str](_out_scalar=__cy.int2hex, ...)Then existing code that imports # basic_int.py
basic_int = _API_FUNCTIONS[int](_out_scalar=_basic_int_id, ...)
methods = {
name: getattr(basic_int, name)
for name in dir(basic_int)
if callable(getattr(basic_int, name)) and not name.startswith("_")
}
globals().update(methods)But I think pylint and other linting tools should be able to see the methods inside the reveal_type(basic_int.geo_to_h3(0, 0, 1))
reveal_type(basic_str.geo_to_h3(0, 0, 1))
# temp.py:44: note: Revealed type is "builtins.int*"
# temp.py:45: note: Revealed type is "builtins.str*" |
|
Closing in favor of #213, which is a better approach |
Overview
This PR adds type stubs (
.pyifiles) to declare the types of the h3 bindings. This allows consumers of theh3library to type check their interactions with it.Having type stubs in separate
.pyifiles is beneficial because consumers ofh3can freely opt in to the typing ecosystem if they wish, while theh3runtime code doesn't need to change at all. I.e. this should have no changes for Python 2.7; it's my understanding that.pyifiles are completely ignored except by type checkers, linters, etc.Examples
Here are some examples with using
mypyon this PR. To test locally, install this branch, copy the Python snippet into a temporary file and call mypy on it (e.g.mypy example.py). (I had to use a local copy of mypy within the virtualenv, separated from my global mypy).Basic int
Basic str
Numpy int
Recent versions of numpy have an optional typed API. Note that you need to enable the mypy Numpy plugin to run these tests.
Note that here the input types are also quite restrictive. We could relax the type errors for numeric input if we split the
H3Cellgeneric type intoH3CellInputandH3CellOutputtypes. The input we could allow to be numeric-like but the output would always be of typenp.uint64.Todo:
hex_range_distancesthat returnOrdered[Unordered[H3Cell]]. I wasn't able to get return values ofGeneric[Generic]to workh3_set_to_multi_polygonpolyfilltypes.Literaltype, which is only in the standard library from 3.8+. Someone on Python <3.8 who doesn't use type checking should not be affected. But I need to check the behavior for type checkers on Python 3.6 and 3.7. We might need to importLiteralfromtyping_extensions. So it might be best to add a newextratoextras_requirecalledtypingthat installstyping_extensionson Python <3.8.Other Notes
memoryviewhas a generic typing API. So we could declare types that accept and return memoryviews, but I don't think require memoryviews withuint64data.