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 check #1663

Merged
merged 7 commits into from
Aug 11, 2023
Merged

ci: add typos check #1663

merged 7 commits into from
Aug 11, 2023

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Aug 10, 2023

This PR adds a typo checker CI action that will fail
future PRs if they introduce typos and spelling mistakes.

This supersedes and thus closes #1660.

src/commands/commander.h Outdated Show resolved Hide resolved
torwig
torwig previously approved these changes Aug 10, 2023
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

LGTM

@szepeviktor
Copy link
Contributor

szepeviktor commented Aug 10, 2023

Currently "Install typos-cli" step takes 2-3 minutes 😲

This takes 1 second 🐎
https://github.com/szepeviktor/byte-level-care/blob/b2e8a838c14be862f95f1bb3486d3b2479b1bc79/.github/workflows/spelling.yml#L34-L46

@tisonkun
Copy link
Member Author

tisonkun commented Aug 10, 2023

Currently "Install typos-cli" step takes 2-3 minutes 😲

I saw similar trick in OpenDAL. Let me try it out.

Actually we may make use of Rust GHA cache. But it's an action beyond the approved ones so I may postpone a bit for using it.

@tisonkun
Copy link
Member Author

tisonkun commented Aug 10, 2023

@szepeviktor I found that typos upstream provides an action already and thus I'm making use of it :D

Maybe you can switch to this solution also.

@szepeviktor
Copy link
Contributor

szepeviktor commented Aug 10, 2023

Maybe you can switch to this solution also.

@tisonkun Here are some facts for you.

  • that action is very complicated - but you see only 2 lines in your workflow
  • that action works on the filesystem, not on git index
  • that action runs typos twice

These made me rewrite it in 6 commands.

@tisonkun
Copy link
Member Author

@szepeviktor thanks for your advice. Also the action isn't approved by INFRA yet. May you create a pr onto my with your installation solution? That is, open a PR against this one, Or use suggestions.

@szepeviktor
Copy link
Contributor

szepeviktor commented Aug 10, 2023

I am sorry. I cannot contribute here.

My solution needs hard-wired excludes etc.

@PragmaTwice
Copy link
Member

PragmaTwice commented Aug 10, 2023

ASF repo does not allow to use action like crate-ci/typos@master. Maybe you need to specify the commit hash or version tag, like @1234567 or @1.0, which is allowed.

@tisonkun
Copy link
Member Author

@PragmaTwice No. It's the action itself not approved, not the version the issue.

So we can either ask INFRA to approve or use a non-action solution. I'm going to reuse @szepeviktor's solution in https://github.com/szepeviktor/byte-level-care under MIT license.

@tisonkun
Copy link
Member Author

Or just OpenDAL's for simplicity.

@tisonkun
Copy link
Member Author

enjoy-binbin
enjoy-binbin previously approved these changes Aug 11, 2023
git-hulk
git-hulk previously approved these changes Aug 11, 2023
PragmaTwice
PragmaTwice previously approved these changes Aug 11, 2023
tisonkun and others added 3 commits August 11, 2023 13:59
@enjoy-binbin enjoy-binbin merged commit f432e6c into apache:unstable Aug 11, 2023
26 checks passed
@tisonkun tisonkun deleted the typos branch August 11, 2023 07:27
p1u3o pushed a commit to p1u3o/incubator-kvrocks that referenced this pull request Aug 15, 2023
This PR adds a typo checker CI action that will fail
future PRs if they introduce typos and spelling mistakes.

This supersedes and thus closes apache#1660.

Signed-off-by: tison <[email protected]>
Co-authored-by: Viktor Szépe <[email protected]>
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.

6 participants