Add a stricter config pass for pyright #5612
Conversation
| (.venv3)$ python3 tests/pyright_test.py stdlib/sys.pyi # Check one file | ||
| (.venv3)$ python3 tests/pyright_test.py # Check all files | ||
| (.venv3)$ python3 tests/pyright_test.py stdlib/sys.pyi # Check one file | ||
| (.venv3)$ python3 tests/pyright_test.py -p pyrightconfig.stricter.json # Check with the stricter config. |
There was a problem hiding this comment.
We should document somewhere (not necessarily here) what the "stricter config" is and why it exists.
There was a problem hiding this comment.
Where would you suggest? I can leave a comment at the top of the stricter config, or put it somewhere else.
There was a problem hiding this comment.
What's the purpose of checking a single file?
tests/README.md seems like a good location to document this.
There was a problem hiding this comment.
It can be useful; I didn't add this instruction though (it was already there).
I'll stick something in that readme.
| version: ${{ env.PYRIGHT_VERSION }} | ||
| python-platform: ${{ matrix.python-platform }} | ||
| python-version: ${{ matrix.python-version }} | ||
| no-comments: ${{ matrix.python-version != '3.9' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy. |
There was a problem hiding this comment.
Maybe we should make only one of the strict and the lenient run generate comments?
There was a problem hiding this comment.
If the first one fails, then it will leave comments and the next step will not run. If the first one passes, then it won't leave any comments and the second one will continue as normal. There shouldn't end up being any duplication.
pyrightconfig.stricter.json
Outdated
| "typeshedPath": ".", | ||
| "include": [ | ||
| "stdlib", | ||
| // Modules that pass this stricter mode. |
There was a problem hiding this comment.
Wouldn't it be preferable to exclude the modules that don't pass the strict checks? That way, there's added incentive to remove them from the exclude list.
There was a problem hiding this comment.
Sure, I can do it that way.
There was a problem hiding this comment.
Flipped it; now it's like the old config.
There was a problem hiding this comment.
I don't think opt-out is a good idea, at least for third-party packages. I predict that this will cause CI to fail for most new packages. But let's wait and see. We can always flip this back later if it indeed turns out to be a common problem.
|
Thanks! I think this is the best of both worlds: Preventing regression of already "complete" stubs and ensuring a higher quality of incomplete stubs (by running pyright against all stubs, including its very useful warnings). |
Fixes #5607.
This adds a second stricter pyright config and runs it first in CI. I've added an example command to the contributing file as well (the test script just passes through its args).
Also bumps pyright, since that hasn't been done in a while.
@erictraut @srittau