jazzband / geojson

Python bindings and utilities for GeoJSON
https://pypi.python.org/pypi/geojson/
BSD 3-Clause "New" or "Revised" License
898 stars 120 forks source link

Type annotation implementation #203

Open am1ter opened 1 year ago

am1ter commented 1 year ago

Initially, my plan was to add features that I needed in my project. However, upon opening the source code, I realized that it was difficult to understand due to the lack of type annotations. Consequently, I decided to focus on adding type annotations to the codebase. While I am not very proficient in refactoring an existing library, I hope that you can provide me with advice on how to improve my commits or even take on the task yourself.

For this task, I used MyPy as a static type checker, which helped me ensure consistency in my annotations. Although I did not intend to refactor the codebase while adding type annotations, there were a few instances where I had to do so to resolve MyPy errors.

In addition to adding type annotations, I also created a new .py file containing all the type variables and aliases. I primarily used type aliases instead of custom types because refactoring to use new custom types would be more complex and dramatic. However, I believe it would be beneficial to consider this approach in the future.

I also made some minor changes to comments and support files.

I am confident that my refactoring did not impact any library functions. All tests passed on Python versions 3.7 through 3.11, as well as in PyPy3. Unfortunately, certain features cannot be implemented due to the need to support Python 3.7, which does not support Literals and isinstance() for Protocols.

P.S. MyPy and Flake8 checks also passed.

am1ter commented 1 year ago

Hello, @am1ter. Thank you for the PR. I'm not a maintainer, but I found a related issue: #167. And, I recommend you to create small separate PRs for fixing irrelevant issues.

Hello @NoharaMasato! Thank you for you reply.

  1. I haven't seen issue #167. Now that I have read it, I would like to invite @kylebarron and @trcmohitmandokhot to take a look at my PR. Perhaps they could review it or even consider contributing to this library.
  2. Before I started working on my PR, I had a similar idea as @kylebarron. However, I quickly realized that it would be quite challenging because the library is designed to be very flexible and work with different types of data. Consequently, it would need to be reworked dramatically.
  3. I think that the refactoring I mentioned above could be done, but it is a significant task. We would not only need to rework the library itself but also some unit tests. Honestly, I am not sure that it could be done without breaking changes. Therefore, I decided to annotate the library as-is without significant refactoring. From my perspective, it is currently easier to understand how it works, which could simplify future improvements.
  4. I am sorry, but for me, it is quite challenging to imagine how I could split my PR into smaller ones. It was a complex task, and I was forced to work with multiple files simultaneously. I would appreciate practical advice.
kylebarron commented 1 year ago

I don't use this package often, but I took a cursory look and it looks great.

trcmohitmandokhot commented 11 months ago

@am1ter - Thank you for the commit. My apologies for getting to this so late. I am very new to implemting Typing in Python, but overall the types.py feels good to me. I will review it some more and ask a few questions! This looks like an impressive and comprehensive effort!

am1ter commented 11 months ago

@trcmohitmandokhot Thank you for the review. By the way, the changes in this PR are so huge, a review from @rayrrr as the maintainer of this repository is mandatory.

rayrrr commented 8 months ago

@am1ter thank you for your efforts here, and apologies for the delayed review. It was your statement in the description...

certain features cannot be implemented due to the need to support Python 3.7

..which gave me pause. Which features?

Notably, in the intervening time since you originally submitted this PR, Python 3.7 has been EOL'ed. I propose that we drop 3.7 support as part of this type annotation implementation, and implement whatever features were blocked. @am1ter would you be able to handle that?

trcmohitmandokhot commented 7 months ago

I am trying to connect the dots and answer @rayrrr 's question "which features cannot be implemented" if Python 3.7 compatibility is to be retained. My read of the situation is that two things change on the typing library between Python 3.7 and 3.8.

  1. PEP 586 was introduced in Python 3.8. This meant that a "Literal" type was added to the fold, which is one of the "typing library features" that is not available in Python 3.7.
  2. The other change is that the typing library from Python 3.8 onwards allows for some custom protocols to be defined runtime_checkable
    As long as @am1ter's PR provides type-checking without relying on these Python 3.8+ typing library features, and that we are okay with not having a weak runtime checkable functionality, this PR could still be considered valid.
am1ter commented 7 months ago

Hello @rayrrr, @trcmohitmandokhot.

Thank you for your responses here. I've had a very busy two weeks, so I apologize for the late response.

Regarding your question about type-related features that Python 3.7 doesn't support, @trcmohitmandokhot's response is correct. As I recall, I mentioned in my description what bothered me in Python 3.7 limitations: Python 3.7 doesn't support using the isinstance() function with Protocols. Consequently, I was forced to use unclear isinstance checks with raw types like isinstance(number, (Real, Decimal)), define a custom SupportRound type alias used only in some cases, and I wasn't able to define custom Protocols for more efficient runtime type checking.

@rayrrr, if you like the main idea of this PR and approve of dropping Python 3.7 support, I believe I can update the PR accordingly and update resolve all merge conflicts.

rayrrr commented 7 months ago

@am1ter yes, I approve of the main idea of this PR and dropping 3.7 support. Please update the PR as you stated and we can go forward from there. Thanks for your efforts so far.

aj-hitchins commented 7 months ago

Hi @am1ter, you might want to consider adding a py.typed file to mark that this package supports typing. See PEP-0561. Hope it helps.