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

Type annotation implementation #203

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Type annotation implementation #203

wants to merge 2 commits into from

Conversation

am1ter
Copy link

@am1ter am1ter commented Apr 7, 2023

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.

Fix bugs with wildcard imports
Use context manager for file reading in `setup.py`
Update tox.ini
Update license copyright years
Add module `types.py` with custom types
Add type annotations to geojson functions
Refactor some functions
Update functions docstrings
@am1ter
Copy link
Author

am1ter commented Apr 13, 2023

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 Feature request: Static type support using TypedDict #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
Copy link

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


# Python 3.7 doesn't support using isinstance() with Protocols
# Use checks for (Real, Decimal) instead
SupportsRound: TypeAlias = Union[Real, Decimal]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we review #188 to see if type checking against Numbers is a better idea than Real or Decimal? This is more in alignment with PEP 3141 and the discussion in #173

Copy link
Author

Choose a reason for hiding this comment

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

Hello, thank you for your comment. It's been a while since I suggested this PR, but if I remember correctly, I considered the possibility of using Number instead of the Real-Decimal combination. Please correct me if I am wrong, but as I understand, some members of the Number subset do not support the SupportsRound protocol (for example, Complex). So, it is impossible to use them in this case.
The best solution is to use the SupportsRound protocol from the typing library, but it is only possible in the case of dropping support for Python 3.7.

Copy link
Member

Choose a reason for hiding this comment

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

Reopening this as a reminder to implement typing.SupportsRound here as part of dropping Python 3.7 support.

Copy link
Member

Choose a reason for hiding this comment

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

Also, to be clear, @am1ter, please use Number as per #188 as @trcmohitmandokhot suggested. Let's not worry about the edge case of complex numbers to whatever extent that might still be a factor.

@trcmohitmandokhot
Copy link
Contributor

@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
Copy link
Author

am1ter commented Aug 3, 2023

@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
Copy link
Member

rayrrr commented Nov 1, 2023

@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
Copy link
Contributor

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
Copy link
Author

am1ter commented Nov 19, 2023

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
Copy link
Member

rayrrr commented Nov 19, 2023

@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
Copy link

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.

@rayrrr rayrrr mentioned this pull request Nov 22, 2023
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.

5 participants