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 spellchecking to pre-commit hooks and linting workflow #92

Merged
merged 28 commits into from
Jul 19, 2023
Merged

Conversation

eliykat
Copy link
Member

@eliykat eliykat commented Apr 21, 2023

Objective

Add spellchecking, using cspell. Because we all make typos.

Code changes

  • install cspell as a dev dependency
  • add configuration in cspell.yaml, largely following the documentation
  • add custom word list
  • add npm script, add to lint-staged, and add to Github linting workflow
  • the good part: fix typos 😁

@eliykat eliykat requested review from MGibson1 and Hinton as code owners April 21, 2023 01:24
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 21, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d45de8c
Status: ✅  Deploy successful!
Preview URL: https://14535467.contributing-docs.pages.dev
Branch Preview URL: https://cspell.contributing-docs.pages.dev

View logs

@eliykat
Copy link
Member Author

eliykat commented Apr 21, 2023

I'm having issues with glob pattern matching in npm scripts, probably because of this issue. npm run spellcheck doesn't include all files, whereas running the command from the commandline using npx does. lint-staged should still work correctly though.

Will look into this next week.

@eliykat eliykat added the hold do not merge label Apr 21, 2023
@withinfocus
Copy link
Contributor

I happened to come upon this, and I have an online linter proposed in our template repository at bitwarden/template#2. I actually disabled cspell based on past experience where it's really unwieldy to maintain. You can see some alternatives that the Mega-Linter uses at https://megalinter.io/latest/descriptors/spell/.

@eliykat
Copy link
Member Author

eliykat commented Apr 24, 2023

Thanks @withinfocus. I see that MegaLinter can run locally, maybe we can add pre-commit hooks to your template PR, and then implement it here once we have the pattern established. (I still think pre-commit hooks are valuable for immediate feedback to the dev, whereas CI checks are more like the final check.) What do you think?

@withinfocus
Copy link
Contributor

CI vs. local linting is definitely the most contentious part of that proposed change. I haven't personally executed it locally but will give it a try to see its performance -- my concern is its heaviness and speed despite its power.

Separately though it may be valuable to avoid cspell.

@djsmith85 djsmith85 mentioned this pull request Apr 25, 2023
@MGibson1 MGibson1 removed their request for review May 4, 2023 20:00
@eliykat eliykat mentioned this pull request May 30, 2023
This reverts commit 7069203.
@eliykat eliykat changed the base branch from master to fix-typos May 30, 2023 01:54
@eliykat
Copy link
Member Author

eliykat commented May 30, 2023

Typo fixes have been moved to a separate PR, #129. I haven't had a lot of time to progress this one but I'll come back to it soon.

Base automatically changed from fix-typos to master May 30, 2023 22:23
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix the linting errors and this should be good.

eliykat added 3 commits July 17, 2023 12:46
This is because npm lifetime scripts (such as npm run) execute in
bash 3.2 on OSX by default, and you need bash 4 for globstar (**).
Quotes prevent the shell from interpeting it and ensure it is passed
to the program itself
@eliykat eliykat requested review from a team as code owners July 17, 2023 03:27
@bitwarden-bot
Copy link

bitwarden-bot commented Jul 17, 2023

Logo
Checkmarx One – Scan Summary & Detailsc9d22be7-bdfe-4447-bbc9-055b8b118598

No New Or Fixed Issues Found

@eliykat eliykat requested a review from Hinton July 17, 2023 03:31
Copy link
Member

@vgrassia vgrassia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow portion looks good to me.

@eliykat eliykat removed the hold do not merge label Jul 17, 2023
@eliykat eliykat merged commit 212c853 into master Jul 19, 2023
@eliykat eliykat deleted the cspell branch July 19, 2023 01:04
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.

5 participants