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

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from
Draft

Conversation

StefanBrand
Copy link
Contributor

Ref #1125

  • Initial type stubs were generated using Instagram/MonkeyType
  • Manual fixes were applied until there were no errors
  • For mypy, some dependencies are ignored in pyproject.toml
  • Pipeline was added to Github Actions

The type stubs still contain many Anys. Now we would really like to gather feedback to make the type annotations stricter, especially around the important places:

Being able to static type check writerecords() or the collection iterator would probably be the most useful thing

@StefanBrand StefanBrand mentioned this pull request May 11, 2023


def eval_feature_expression(
feature: Dict[str, Union[Dict[str, Union[str, List[List[List[float]]]]], str, Dict[str, Optional[Union[float, str, int]]]]],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For complex combinations of basic types, it's probably useful to introduce names for (sub)expressions.

I like the style that the httpx library uses:
https://github.com/encode/httpx/blob/master/httpx/_client.py#L1111


def parse_time(
text: str
) -> Union[Tuple[int, int, int, int, int, int, int, float], Tuple[int, int, int, int, int, int, int, None], Tuple[int, int, int, int, int, int, int, int]]: ...
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also benefit from at least an alias.

@sgillies
Copy link
Member

@StefanBrand thank you for explaining the approach! I had not heard about MonkeyType.

Did adding type stubs find any Fiona bugs?

It will be a while before I can give my full attention to a review of this. The Fiona 1.9.4 release and some wheel building updates are higher priority for me, as are the next Rasterio release and some Shapely project governance work.

@StefanBrand
Copy link
Contributor Author

@sgillies Under my impression the type stubs derived from the tests by MonkeyType are still quite broad. We would now have to make the types stricter here and there to catch bugs if there are any. My approach would be to more correctly represent the __geo_interface__ / GeoJSON spec. We can take inspiration from https://github.com/developmentseed/geojson-pydantic, as explained in this comment: jazzband/geojson#167 (comment)

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

5 participants