Typechecking improvements #1738
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cspotcode
force-pushed
the
typecheck-ci
branch
5 times, most recently
from
7ff6dc0 to
c86440d
Compare
…generated convenience makefile and justfile
|
4k lines of errors from pyright, not surprising since I configured it in strict mode and then disabled a few checks that were not really things we could reasonably fix. https://github.com/pythonarcade/arcade/actions/runs/4839729643/jobs/8624935183?pr=1738 If anyone feels like taking a look, maybe some of those diagnostics we want to fix in follow-up PRs, and others we disable? |
|
Some low-hanging fruit we could address: |
pushfoo
reviewed
Apr 30, 2023
There was a problem hiding this comment.
tl;dr "don't edit this" comments at the top of a file indicate a 100% chance someone will try to edit it
Although making the file clearly grammatically invalid (nothing but comments) can work, it seems better to take this approach:
- Don't have the original in the git history, at least not under its final name
- Add a linting check to fail build any time someone tries to add it
The downside is this increases build process complexity. I'm already worried about this aspect of the PR, but I'll hold off on approving or requesting changes until I understand its pros and cons better.
| @@ -0,0 +1,101 @@ | |||
| # DO NOT EDIT BY HAND: is automatically re-generated by make.py | |||
There was a problem hiding this comment.
Same as the comment on Makefile: if a line like this has to exist, it seems like a problem.
There was a problem hiding this comment.
We could just .gitignore them. So anyone who wants the convenience of make will get it right after they run make.py for the first time.
The only benefit of versioning it in git is that it exists before the first time you run make.py. After that, you're constantly re-running make.py -- even via make -- which is constantly re-emitting the makefile.
| @@ -0,0 +1,133 @@ | |||
| # DO NOT EDIT BY HAND: is automatically re-generated by make.py | |||
There was a problem hiding this comment.
I'm not sure about this.
We had auto-generated parts before, specifically __init__.py. If I remember correctly, we removed auto-generation in large part due to constant confusion over what needed to be updated.
pyrightconfig.json
Outdated
There was a problem hiding this comment.
GitHub is flagging all comments as bright red. To my understanding, the JSON spec does not allow for comments, even if specific parser implementations do.
Is there an alternate config format allowed?
There was a problem hiding this comment.
I went with pyrightconfig.json because VSCode tab-completes all the fields, unlike with pyproject.toml. The experience of setting it up was much nicer this way. Unfortunately a lot of tools these days use the JSON extension but actually parse it as JSONC. I've added a .gitattributes directive telling Github that pyrightconfig.json files are always JSONC.
There was a problem hiding this comment.
I tried opening the project in PyCharm and it doesn't seem to understand pyrightconfig.json the way VSCode does. So I can move the config into pyproject.toml, I have the changes locally but would like to wait till I get some feedback on if we actually want any of these stricter typechecking rules enables.
| def emit_makefile(): | ||
| with cd_context(PROJECT_ROOT): | ||
| with open('Makefile', 'w') as f: | ||
| f.write('# DO NOT EDIT BY HAND: is automatically re-generated by make.py\n') |
There was a problem hiding this comment.
If we're going this route, it may make more sense to also paste above every directive inside the loop. That still might not be enough.
This was referenced May 3, 2023
Merged
|
Closing since everything here has been extracted to other PRs and merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes several things, some of which might be rejected. But they're related closely enough, I thought it easier to open one PR for feedback, then modify to remove the stuff that doesn't pass review.
adds pyright typechecking
make pyrightConfigured by
pyrightconfig.json. The config can technically go intopyproject.toml, but the json config has nice editor tab-completion which makes it easier to tweak.Adds auto-generated
MakefileandjustfileProbably we don't want both of these, but just for sake of demonstration. When
make.pystarts, it automatically re-writes these files. Then they're committed to git. So they will only change if you yourself have made changes tomake.py, and then you can commit the changes to git.Nice thing about justfile: you can be in any directory and type
just lint. It will always run commands from the project root.Splits lint logs on CI
ruff, mypy, and pyright logs appear as 3x separate sections in Github Actions results. I wired it up so that all 3 will always run even if an earlier one fails.
make.py --helporganized into sectionsA nice feature of Typer: you can organize the subcommands into sections. I split them into
DocsandCoding; that name's not great. MaybeDev?Reviewing pyright diagnostics
If you're using VSCode and want to review the outstanding VSCode diagnostics to decide which we fix and which we disable, you can use this
.vscode/settings.jsonThen open the Problems pane and sort by the diagnostic you want to see.