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

Initial pyright config #4192

Merged
merged 18 commits into from
Aug 27, 2024
Merged

Initial pyright config #4192

merged 18 commits into from
Aug 27, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Jan 25, 2024

Summary of changes

Work towards step 2 of #2345 (comment) . Non-trivial fixes should go in different PRs.
Merge #3979 and #4257 first.
npx -y pyright@latest . --pythonversion=3.8|3.12 --pythonplatform=Linux|Windows results:
0 errors, 418 warnings, 0 informations

Pull Request Checklist

.github/workflows/main.yml Outdated Show resolved Hide resolved
@Avasam Avasam changed the title Pyright initial config Initial pyright config and turn on type-checking in CI Jan 25, 2024
@@ -138,3 +143,4 @@ def get_output_mapping(self) -> Dict[str, str]:
Destination files should be strings in the form of
``"{build_lib}/destination/file/path"``.
"""
...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Python should not require ellipsis when the docstring is provided. I am not 100% inline with the comment of the pyright maintainer... The reason why the linters don't complain about it, is because there is nothing wrong 😅

Does mypy also complains about it?
Should we just stick with mypy for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the problem here is because we are talking specifically about prototypes, and the spec says:

Bodies of protocol methods are type checked.

That is a bit disappointing... These pesky ellipsis can be annoying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Further discussion: astral-sh/ruff#8756 (comment) . It does seem to be more accurate to spec. Ruff was also updated to not strip ellipses in Protocols in https://github.com/astral-sh/ruff/releases/tag/v0.1.6

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Hi @Avasam , thank you very much for the efforts in this field. I understand that this is a wipe, but I left a few comments, hopefully that is fine with you.

@@ -138,3 +143,4 @@ def get_output_mapping(self) -> Dict[str, str]:
Destination files should be strings in the form of
``"{build_lib}/destination/file/path"``.
"""
...
Copy link
Contributor

Choose a reason for hiding this comment

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

Python should not require ellipsis when the docstring is provided. I am not 100% inline with the comment of the pyright maintainer... The reason why the linters don't complain about it, is because there is nothing wrong 😅

Does mypy also complains about it?
Should we just stick with mypy for now?

setuptools/command/easy_install.py Outdated Show resolved Hide resolved
@Avasam
Copy link
Contributor Author

Avasam commented Jan 25, 2024

I understand that this is a [wip] but I left a few comments, hopefully that is fine with you.

Perfectly fine! I'm just waiting for a merge on #3979 (there's some overlap) before publishing the PR.
I especially need guidance on running type-checkers (mypy/pyright) on the CI (tox/pytest setup) to prevent regressions.

@Avasam Avasam mentioned this pull request Mar 5, 2024
2 tasks
@Avasam Avasam changed the title Initial pyright config and turn on type-checking in CI Initial pyright config Mar 6, 2024
@Avasam Avasam force-pushed the pyright-initial-config branch 2 times, most recently from 872fc45 to 257efa3 Compare March 6, 2024 21:07
@Avasam

This comment was marked as resolved.

@Avasam Avasam marked this pull request as ready for review March 8, 2024 05:21
mypy.ini Outdated Show resolved Hide resolved
mypy.ini Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
pyrightconfig.json Outdated Show resolved Hide resolved
pyrightconfig.json Show resolved Hide resolved
setuptools/config/_apply_pyprojecttoml.py Outdated Show resolved Hide resolved
tox.ini Outdated
@@ -6,6 +6,7 @@ setenv =
PYTHONWARNDEFAULTENCODING = 1
SETUPTOOLS_ENFORCE_DEPRECATION = {env:SETUPTOOLS_ENFORCE_DEPRECATION:1}
commands =
pyright --threads
Copy link
Contributor

Choose a reason for hiding this comment

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

I am really sorry @Avasam, I had a change of heart at this.

It seems that when you install pyright, it automatically install NodeJS. That is a huge undertake: it is a separated programming language/environment and it does seem excessive to automatically add that to the contributors' machines (or mine).

In this scenario, I am reconsidering the first approach that you suggested (adding a new workflow to the CI). That might work better.

I am thinking that if we have a separated workflow file, it would not interfere with skeleton. That might be the best scenario.

Copy link
Contributor Author

@Avasam Avasam Aug 22, 2024

Choose a reason for hiding this comment

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

I am reconsidering the first approach that you suggested (adding a new workflow to the CI). That might work better.
I am thinking that if we have a separated workflow file, it would not interfere with skeleton. That might be the best scenario.

I am 100% for this solution and is my preference as well ! I'll re-request your review once that's done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so here's a workflow file without touching tox.ini or .github/workflows/main.yml, or referencing pyright in pyproject.toml.

Incidentally it also won't be picked-up by the collateral job.

@abravalheri abravalheri merged commit d12330d into pypa:main Aug 27, 2024
21 of 23 checks passed
@abravalheri
Copy link
Contributor

Thank you very much, let's get this one going.

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.

2 participants