-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: Add a typos CI job #16122
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
base: master
Are you sure you want to change the base?
feat: Add a typos CI job #16122
Conversation
.github/workflows/spelling.yml
Outdated
- name: Checkout Actions Repository | ||
uses: actions/checkout@v5 | ||
- name: Spell Check Repo | ||
uses: crate-ci/typos@master |
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.
Do we want this to block CI?
If so, we have to do one of
- change settings on the repo
- put this in the main workflow and have Conclusion depend on it
(iiuc)
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 also should pin the job and have renovatebot update it
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.
IMHO it should be a non-blocking job first:
- We don't know how reliable it is (e.g. how many false-positives it has)
- We don't want to block the PR just because some new identifier needs to be added to the typo config
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.
Sounds like these are summarized as "we don't know how much of a pain this will be, are we sure we want to commit?".
However, we do have history for this
- rust-lang/rust has adopted this and you can see how much churn their config has
- I've been using this in all of my repos for years with much stricter configs than cargo and rust
On the other hand, by not having it be blocking,
- We might enable auto-merge and miss that typos has failed
- We confuse contributors when with existing failures
- We confuse contributors with whether they are accountable for it (the UI does not make it clear when jobs are non-blocking)
5b2c0fd
to
bf11cab
Compare
Over the past few months, I have noticed various PRs that were created to fix typos, and figured that it would be a good idea to add a non-blocking1 CI job that runs
typos
2 to hopefully catch typos before they are merged into the codebase. The configuration file fortypos
in this PR is meant to get the ball rolling and is by no means perfect.Footnotes
I chose to make this non-blocking, so any newly added corrections don't cause CI to fail. ↩
The workflow file for the job matches the one in
annotate-snippets-rs
↩