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

ci: add typos as spell checker #641

Merged
merged 8 commits into from
Dec 31, 2024
Merged

Conversation

zyf722
Copy link
Contributor

@zyf722 zyf722 commented Oct 25, 2024

What type of PR is this? (check all applicable)

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert
  • 🌐 Internationalization / Translation

Description

This PR adds crate-io/typos as spell checker in CI process.

Spell errors will cause the CI to fail, with errors reported in the Annotations part of GitHub Actions workflow runs (link to this run):

Its behavior can be controlled with the typos.toml file, with config specifications detailed here.

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentations?

  • πŸ““ docs (./docs)
  • πŸ“• storybook (./storybook)
  • πŸ“œ README.md
  • πŸ™… no documentation needed

Copy link

netlify bot commented Oct 25, 2024

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit 0c1eea0
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/6772d0910ed74000083c7965
😎 Deploy Preview https://deploy-preview-641--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@hatemhosny
Copy link
Collaborator

Thanks @zyf722

That is nice.

Is there an easy way to run the test locally? e.g. npm package. This can allow us to reproduce problems locally without having to keep pushing to GitHub to see results.
I know there is a vscode extension (which is convenient during authoring) but I'm not sure we can use it to run over the whole code base.

How about cspell? Also has a vscode extension which seems to be very popular. I'm actually using it.
(I know I slipped a few spelling mistakes, but I was not enforcing it with a test! πŸ˜…)

@zyf722
Copy link
Contributor Author

zyf722 commented Oct 26, 2024

Is there an easy way to run the test locally? e.g. npm package. This can allow us to reproduce problems locally without having to keep pushing to GitHub to see results.

typos is written in Rust, and as far as I know, there is no official npm package for typos. Instead, you can either manually download pre-built binaries from the GitHub release or install it via cargo, brew, conda, or pacman.

You might also be interested in their pre-commit hook.

How about cspell? It also has a VSCode extension which seems to be very popular. I'm actually using it.

It's new to me. I will check it out and see if it fits our needs better :)

I was not enforcing it with a test! πŸ˜…

Indeed, actually I guess we might be faced with false positives in some cases.

Perhaps it would be better to only throw warning annotations when typos are detected, rather than error annotations and terminate the workflow?

@hatemhosny
Copy link
Collaborator

Indeed, actually I guess we might be faced with false positives in some cases.

Actually, cspell does have some false positives in my experience with it.

Perhaps it would be better to only throw warning annotations when typos are detected, rather than error annotations and terminate the workflow?

I agree.

@hatemhosny
Copy link
Collaborator

@zyf722
Shall we start moving this forward?

It is currently failing for other translations.
Do you think this can be fixed or should we just ignore this directory?

@zyf722
Copy link
Contributor Author

zyf722 commented Dec 27, 2024

@hatemhosny

Thank you for reviewing the PR.

Shall we start moving this forward?

I'd love to! As long as you consider it is worth doing so, I'm always ready to help.

It is currently failing for other translations.
Do you think this can be fixed or should we just ignore this directory?

Already fixed. We previously haven't tracked .lokalise.json files in the repository, so the typos config file didn't exclude them. Now there won't be any more issues with translation files.

@hatemhosny
Copy link
Collaborator

Thank you @zyf722

Perhaps it would be better to only throw warning annotations when typos are detected, rather than error annotations and terminate the workflow?

Currently, the setup would lead to failure of the build workflow and prevent us from merging if typos were detected. (cannot ignore false positives)
If we change it to warning instead of error, the current UI of GitHub PR would lead us to miss them unless we specifically check the workflow logs. (may miss true positives)
I suggest adding it to a new workflow, which if fails it would be visible in the PR UI, but it does not prevent us from merging, similar to how the workflows for sonarcloud and codacy behave.

What do you think?

@zyf722
Copy link
Contributor Author

zyf722 commented Dec 30, 2024

I suggest adding it to a new workflow, which if fails it would be visible in the PR UI, but it does not prevent us from merging, similar to how the workflows for sonarcloud and codacy behave.

Indeed, it's a good idea to add a new workflow for this.

I've moved the spell checking step to a separate file CI-typos.yml which has same trigger conditions as CI-build.

@hatemhosny hatemhosny merged commit 208497b into live-codes:develop Dec 31, 2024
13 checks passed
@hatemhosny
Copy link
Collaborator

Thank you @zyf722
I think this would be very useful.

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.

2 participants