-
Notifications
You must be signed in to change notification settings - Fork 21
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
GitHub Action to run codespell #65
base: main
Are you sure you want to change the base?
Conversation
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 not enthusiastic about adding these to the CI, since it means local tests will no longer match what the CI does without the user having to install these tools.
The codespell one seems OK I guess, as it's cosmetic, but the linter seems like overkill given how little the nearly-throwaway Python scripts in this repo change.
%
|
.github/workflows/unit-tests.yml
Outdated
- uses: actions/checkout@v4 | ||
- uses: codespell-project/actions-codespell@v2 | ||
with: | ||
ignore_words_list: inout,numer,passt,statics |
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.
Hm. If we add a false positive in the future, how will someone know this is where to look for the exceptions list?
I'm definitely sympathetic to the desire for this, and it did find some real fixes, but I'm not convinced this is worth its weight as an ongoing dependency.
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.
We could store config in pyproject.toml
or close this PR.
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.
My concern there is: Given that the Python code here isn't really a "primary" artifact of the repo, I think it would be distracting to litter config files for Python-specific tools (or IDEs, or other ancillary things) into the repo. (Settings for a C linter, on the other hand, would probably be fine, since it's a C library 🙂)
Not that I couldn't be convinced otherwise, but I think I'd rather not add those kinds of files on that basis.
Perhaps a more practical solution would be to add a bit to the README. Why don't we leave the codespell bit hooked up as it is, and I can (separately) add a paragraph there.
a78529e
to
5f2a1a0
Compare
5f2a1a0
to
fbaf9af
Compare
[tool.codespell]
ignore-words-list = "inout,numer,passt,statics" |
These GitHub Action tests would only pass AFTER #63 and #64 (or similar) are merged.