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

com2ann: Convert type comments into Python type hints #2325

Merged
merged 13 commits into from
Sep 5, 2022

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Aug 16, 2022

Description

https://pypi.org/project/com2ann

  • Summary of the change / bug fix.
  • Link # issue, if applicable.
  • Screenshot of the feature or the bug before/after fix, if applicable.
  • Relevant motivation and context.
  • List any dependencies that are required for this change.

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@cclauss cclauss changed the title com2ann: Convert type comments into Python type hint com2ann: Convert type comments into Python type hints Aug 16, 2022
@DidierRLopes DidierRLopes added the enhancement Enhancement label Aug 27, 2022
@piiq
Copy link
Contributor

piiq commented Aug 30, 2022

What is the rationale to go away from using the typing module? The load time penalty is not that high but having 1 way to type hint the variables in the code improves readability through homogeneity by a lot. What am I missing @cclauss?

@cclauss
Copy link
Contributor Author

cclauss commented Aug 30, 2022

@piiq Type hints are moving towards PEP 585 and PEP 604 and pyupgrade automatically makes these changes.

@piiq
Copy link
Contributor

piiq commented Aug 30, 2022

Thanks for pointing to the PEP. Is my understanding correct that:

  1. Union[str,int] changes to str | int
  2. Optional[str] changes to str | None (and the ~str proposal was not approved)
  3. The from __future__ import annotations is going to stay there until the lowest supported version reaches 3.10?

@cclauss
Copy link
Contributor Author

cclauss commented Aug 30, 2022

Yes for 1. and 2. but I think the jury is still out on 3. ... See the footnote https://docs.python.org/3/library/__future__.html

I have found it useful to run pyupgrade on various test files to understand its transformations.

@piiq
Copy link
Contributor

piiq commented Aug 30, 2022

Thanks. In that case I have another question. From your perspective is it possible to move away from typing module at all? Is there an Any annotation or an annotation for Callable?
I'm asking because I am ironically trying to follow the There should be one– and preferably only one –obvious way to do it. guiding principle

@cclauss
Copy link
Contributor Author

cclauss commented Aug 30, 2022

PEP20:

There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.

The Dutch part is significant here. The solution does not need to be too simplistic.

The community is not saying that we should completely abandon the typing module.

Instead, it advocates using pre-existing built-in types and the collections module types where possible. Having both List and list has confused new Python developers and it leads to unnecessary imports (both time complexity and code complexity). Similarly, having both typing.Callable and collections.abc.Callable suffers from the same issues. The proposed approach may take a bit of time to get used to but there is good logic behind this move.

@piiq
Copy link
Contributor

piiq commented Aug 30, 2022

Thanks for sharing your thoughts.
Would you mind updating the CONTRIBUTING.md to mention the change in how types should be used? And we can merge it

@colin99d colin99d merged commit 335a7b9 into OpenBB-finance:main Sep 5, 2022
@cclauss cclauss deleted the com2ann branch September 5, 2022 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants