Add codespell support (config, workflow to detect/not fix) and make it fix few typos#37
Add codespell support (config, workflow to detect/not fix) and make it fix few typos#37yarikoptic wants to merge 6 commits into
Conversation
=== Do not change lines below ===
{
"chain": [],
"cmd": "codespell -w",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
clane9
left a comment
clane9
left a comment
There was a problem hiding this comment.
Thanks @yarikoptic for the PR. I left a few comments that would be nice to get your thoughts on.
One other question, it seems there is a false positive error from the github action on line 35 of .pylintrc. Any idea why?
| @@ -0,0 +1,25 @@ | |||
| # Codespell configuration is within pyproject.toml | |||
| --- | |||
| name: Codespell | |||
There was a problem hiding this comment.
Is there a strong reason for this to be its own workflow rather than folded into ci.yaml?
There was a problem hiding this comment.
could be done so if you like but
- you have already lots of commands in there sequentially and without separate steps -- so one fails (e.g. black) and others already don't run, and here you just see that CI failed but do not know what check exactly
- why don't you use pre-commit and instead list individual calls there? (related: fresh "Address" detected by pre-commit issues #40). Could be done via action or there is even pre-commit service IIRC (I typically do not use it but other projects do)
- having separate workflow or parametrization for it shows up as a separate report here in PR so makes it easier to immediately see what is wrong. They also then run in parallel
With that in mind I would have recommended to move "pre-commit" encoded checks into separate workflow, and leave/rename ci to run only tests but across different python versions (now just 3.8)
You could also encode them all in a single CI workflow file, but then I would also recommend to centralize them actually outside, e.g. in tox.ini and then just sweep through different invocations. Eg. look at https://github.com/con/duct/blob/main/.github/workflows/test.yaml which sweeps through different pythons for testing but also adds runs for type checks and linting.
so -- many ways to skin a cat here ;-)
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| - name: Annotate locations with typos | ||
| uses: codespell-project/codespell-problem-matcher@v1 |
There was a problem hiding this comment.
Is the codespell-problem-matcher necessary? What does it do on top of the basic codespell action?
There was a problem hiding this comment.
it annotates typos directly in the PR diff - quite handy
| # Ref: https://github.com/codespell-project/codespell#using-a-config-file | ||
| skip = '.git*' | ||
| check-hidden = true | ||
| ignore-regex = '(^\s*"image/\S+": ".*|ND)' |
There was a problem hiding this comment.
These are here to suppress false positives? We will see how long this gets 😅.
There was a problem hiding this comment.
I fixed ND to be a complete word now, that is what tripped CI. For the ipynb included images -- quite a reliable pattern , wouldn't expect problems.
… folders I might have
|
Just now recalled about this effort. I provided explanations and pushed some changes to address comments. Could this PR be re-reviewed and verdict made? |
|
Thanks for the PR and reminder @yarikoptic. I think let's take the pre-commit hook, but I'd rather not add to the CI for now. In principle, the CI should be redundant with the pre-commit, as long as the pre-commit is applied consistently. Would you mind removing the added CI workflow and then we can merge? |
|
Codespell added to pre-commit in #48. |
2 participants
More about codespell: https://github.com/codespell-project/codespell .
I personally introduced it to dozens if not hundreds of projects already and so far only positive feedback.
CI workflow has 'permissions' set only to 'read' so also should be safe.