Skip to content
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

Add a CI spell check job for changed files on PRs #1255

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Jan 20, 2025

This PR adds a CI job which will run a cspell check on only the changed files (as compared to master).

As a proof of concept and to show how spell fails (and false positives) should be fixed I've included two files.

image

Some notes:

  • The "bonus" we get when we merge this PR is that typo-PRs will thus now also get ran through the cspell check 😄 👍
  • The check runs in case sensitivity, for instance adding "Beiko" to known words will flag "beiko"
  • I've added area-specific words to the global word list, like EIP, Holesky, etc.
  • To add known words to a markdown file add a "comment" using [//] # cSpell:words WORD_LIST, see: https://stackoverflow.com/a/20885980
  • In the Attendees section of the Meeting, wrap this in (names are obv. flagged as "spell error"):
### Attendees
[//] # cSpell:disable
* <Attendee name 1>
* <Attendee name 2>
...
* <Attendee name N>
[//] # cSpell:enable

Note: cspell defaults to US English which is why "standardisation" is flagged on Meeting-202.

Obviously open for discussion, but I think for now a spell check job on new files would be handy. On meeting-202 it found one obvious error: eaps -> EIPs. Have not checked the other files (besides throwing them in the cspell job, this obviously flags a lot of errors)

@jochem-brouwer
Copy link
Member Author

Hmm ok I had assumed that some warning would pop up that PR-triggered actions are not triggered here, but it seems not. The action does run on my forked repo though: jochem-brouwer#2

@taxmeifyoucan
Copy link
Member

I think this is a great feature for the repo! I am not familiar with cspell but it looks like it does the job, in epf.wiki repository we are using aspell with big wordlist including many Ethereum related terms. Maybe it could be useful here as well https://github.com/eth-protocol-fellows/protocol-studies/blob/main/wordlist.txt

Also one extra function we do is to post a comment on the PR which points out exactly what words are misspelled instead of just failing the job. Might be good to trigger a comment here as well to make it easier to identify which words are causing the issue

@timbeiko
Copy link
Contributor

Thanks for the review, @taxmeifyoucan! @jochem-brouwer I agree that at least having the comment on the PR would be a nice addition for people to easily fix the typos. Could you please add that and then I'll merge this in? Thanks!

@jochem-brouwer
Copy link
Member Author

Great suggestion! I'll add a GH comment in to dump the spelling mistakes (or in some cases false positives). I will also update to use this ethereum-based whitelist for words :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants