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 checker #3116

Merged
merged 9 commits into from
Jan 13, 2025
Merged

Add type checker #3116

merged 9 commits into from
Jan 13, 2025

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Dec 18, 2024

This PR adds type checker to tox.

PyRight Vs MyPy

There's a lot of discussion around about which one to use. I don't think it matters much which one to choose. There are points where Python itself is not specific, and then the type checkers have freedom to choose the behavior they want... Example:

  • mypy: On strict mode, you need to add -> None on the return statement.
  • pyright: infers that is None, so you don't need to add -> None.

PyRight is easier for VSCode users, me included... 👀

About the ecosystem... I think most of them use mypy, but Pydantic for example, uses PyRight.

If you look at the projects I maintain (starlette, uvicorn), I use mypy on them, but I have to say that is a bit more practical to have the type checker match my IDE.

Anyway... I hope this is not an issue, and we can start with this setup.

@Kludex Kludex requested a review from a team as a code owner December 18, 2024 09:19
dev-requirements.txt Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@Kludex
Copy link
Contributor Author

Kludex commented Dec 18, 2024

cc @emdneto (asked about this on PM) and @lzchen (since you been happy with the type hints PRs 👀 👍)

@xrmx
Copy link
Contributor

xrmx commented Dec 18, 2024

I don't have any preference but I think you missed that we are using mypy in opentelemetry-python

tox.ini Show resolved Hide resolved
@Kludex
Copy link
Contributor Author

Kludex commented Dec 18, 2024

I was checking opentelemetry-python, and it seems that pyright is also present there?

https://github.com/open-telemetry/opentelemetry-python/blob/17782492f5cec2e093fcd02e983d7f5e638b7cca/tox.ini#L349-L358

A bit more into it... Seems like if pyright looks at the pyproject, the type checker is disabled... 🤔

https://github.com/open-telemetry/opentelemetry-python/blob/17782492f5cec2e093fcd02e983d7f5e638b7cca/pyproject.toml#L5-L12

@Kludex
Copy link
Contributor Author

Kludex commented Dec 18, 2024

I took some time trying to set up mypy to see how that would look like, and it just took too much of my time, and I was not able to make it work properly...

I'm happy to close this if people are strong on mypy, but I'm not willing to spend more time trying it.

@Kludex
Copy link
Contributor Author

Kludex commented Dec 20, 2024

I think the mypy setup on opentelemetry-python is not taking in consideration all local packages.

It would be great if people could share opinions on this, I want to continue adding type hints, but without regressions.

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

Thanks. Discussed this at SIG (01/09) and agreed to move forward with pyright in contrib and include some packages to be typechecked. After that, also adopt pyright in core as well.

nit: can we include the packages you've added type hints?

@Kludex
Copy link
Contributor Author

Kludex commented Jan 10, 2025

Not yet, cause I've mainly added type hints on public APIs. I need to add more stuff to them that I was not very strict.

dev-requirements.txt Outdated Show resolved Hide resolved
Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com>
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Very exciting, thank you!

.github/workflows/misc_0.yml Outdated Show resolved Hide resolved
@emdneto emdneto added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 13, 2025
emdneto and others added 2 commits January 13, 2025 11:11
Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com>
@lzchen lzchen merged commit 406707b into open-telemetry:main Jan 13, 2025
675 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants