-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Initial pyright config #4192
Conversation
6450313
to
e7c0804
Compare
@@ -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"``. | |||
""" | |||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to microsoft/pyright#6487
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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"``. | |||
""" | |||
... |
There was a problem hiding this comment.
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?
Perfectly fine! I'm just waiting for a merge on #3979 (there's some overlap) before publishing the PR. |
bbb379f
to
d9f74a7
Compare
872fc45
to
257efa3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
257efa3
to
6519c8d
Compare
cc9d481
to
b33d4a0
Compare
c957935
to
6c7ccd4
Compare
53fd1aa
to
5259c21
Compare
5259c21
to
bbaada8
Compare
bbaada8
to
c4f5c25
Compare
tox.ini
Outdated
@@ -6,6 +6,7 @@ setenv = | |||
PYTHONWARNDEFAULTENCODING = 1 | |||
SETUPTOOLS_ENFORCE_DEPRECATION = {env:SETUPTOOLS_ENFORCE_DEPRECATION:1} | |||
commands = | |||
pyright --threads |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…tuptools into pyright-initial-config
Co-authored-by: Anderson Bravalheri <[email protected]>
Thank you very much, let's get this one going. |
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
newsfragments/
.(See documentation for details)