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

Check typos in CI #1654

Merged
merged 6 commits into from
Aug 13, 2024
Merged

Check typos in CI #1654

merged 6 commits into from
Aug 13, 2024

Conversation

storopoli
Copy link
Contributor

@storopoli storopoli commented Jul 25, 2024

typos
is a powerful source code spell checker.

  • Adds a CI job that runs on every PR and
    push to master (but can also be run manually with
    workflow_dispatch) that checks for typos.

  • Adds a config file .typos.toml that deals with
    false positives and only checks for top-level/one-level
    .mediawiki and .md files. This config can be extended
    quite easily with actual terms or regexes to whitelist
    (false-positives)

  • Fixes some typos in several BIPs that were flagged locally by typos.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Concept ACK

.github/workflows/typos.yml Outdated Show resolved Hide resolved
@storopoli storopoli requested a review from jonatack July 26, 2024 12:00
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

(but can also be run manually with workflow_dispatch)

Can you provide a cross-platform command to run this check manually?

.github/workflows/typos.yml Outdated Show resolved Hide resolved
@storopoli
Copy link
Contributor Author

storopoli commented Jul 26, 2024

(but can also be run manually with workflow_dispatch)

Can you provide the cross-platform command to run this check manually? (I tried with macOS)

Install typos and then run from the root dir:

typos ./*.mediawiki ./*.md ./**/*.mediawiki ./**/*.md

EDIT: add a typo to any md or mediawiki file and see it being flagged

@jonatack
Copy link
Member

Thanks! Tested.

Install typos and then run from the root dir:

typos ./*.mediawiki ./*.md ./**/*.mediawiki ./**/*.md
  • Would you please update the PR description with that documentation? (Separate from this PR, it may be a good idea to add a CONTRIBUTING file, or a section to the readme, that documents how to run the CI checks manually.)

  • Would it make sense to add this task into github-action-checks.yml, and/or make the CI output more like the other tasks:

@@ -1,4 +1,4 @@
-name: Check Typos
+name: GitHub Actions Check
 on:
   push:
     branches:
@@ -8,6 +8,7 @@ on:
 
 jobs:
   typos:
+    name: "Spelling Checks"
     runs-on: ubuntu-latest
     steps:
     - name: Checkout Actions Repository

@storopoli storopoli closed this Jul 28, 2024
@storopoli storopoli reopened this Jul 28, 2024
@storopoli
Copy link
Contributor Author

  • Would you please update the PR description with that documentation? (Separate from this PR, it may be a good idea to add a CONTRIBUTING file, or a section to the readme, that documents how to run the CI checks manually.)

Done, added a CONTRIBUTING.md in ce4351d.

  • Would it make sense to add this task into github-action-checks.yml, and/or make the CI output more like the other tasks:

Good idea! I've removed the new typos.yml and added the job in the already existing github-action-checks.yml in ce4351d.

@storopoli storopoli requested a review from jonatack July 28, 2024 02:03
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for proposing this and putting in the work!

bip-0152.mediawiki Show resolved Hide resolved
@murchandamus
Copy link
Contributor

@jonatack: could you take another look whether your change requests have been satisfactorily addressed? I’m not particularly familiar with CI stuff, so please feel free to merge if you feel confident that it’s ready.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

WDYT about simplifying running the linter with something like the following:

diff --git a/.github/workflows/github-action-checks.yml b/.github/workflows/github-action-checks.yml
index 3f1fce9..17f19c1 100644
--- a/.github/workflows/github-action-checks.yml
+++ b/.github/workflows/github-action-checks.yml
@@ -29,5 +29,3 @@ jobs:
     - name: Check spelling
       uses: crate-ci/typos@master
-      with:
-        files: ./*.mediawiki ./*.md ./**/*.mediawiki ./**/*.md
diff --git a/.typos.toml b/.typos.toml
index e645929..e81f41c 100644
--- a/.typos.toml
+++ b/.typos.toml
@@ -27,3 +27,5 @@ Atack = "Atack"
 Meni = "Meni"
 
+[files]
+extend-exclude = ["/*/*.csv", "/*.d*", "/*/*.d*", "/*/*.go", "/*/*.json", "/*/*/*.json", "/*/*.mod", "/*/*.proto", "scripts", "/*/*.s*", "/*/*.t*"]
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 506a295..df6d947 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -8,5 +8,5 @@ install [`typos`](https://github.com/crate-ci/typos)
 and then run in the root directory:
 
-typos ./*.mediawiki ./*.md ./**/*.mediawiki ./**/*.md
+typos

This seems to work better for me locally and it also allows debugging the linter with its built-in options like typos --files.

Edit: about the commit order, it seems best to fix the typos before adding the linter, or in the same commit, as otherwise the commits after adding the linter are non-hygienic.

@storopoli
Copy link
Contributor Author

Great suggestions! Thanks! I incorporated them all. I also took the liberty of adding .py files since we are not typo checking the scripts for now.

@storopoli storopoli requested a review from jonatack August 7, 2024 22:08
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

@storopoli looks good to me (thanks!) A few nits follow. Let me know if you'd like to update for them or merge this pull as-is.

Edit: also suggest doing the spelling fixups in a single commit.

.typos.toml Outdated Show resolved Hide resolved
.typos.toml Show resolved Hide resolved
.github/workflows/github-action-checks.yml Outdated Show resolved Hide resolved
typos is a powerful source code spell checker.

Adds a CI job that runs on every PR and
push to master (but can also be run manually with
workflow_dispatch) that checks for typos.

Adds a config file .typos.toml that deals with
false positives and only checks for top-level/one-level
.mediawiki and .md files.
@storopoli
Copy link
Contributor Author

Addressed all the nits, sorry for the delay...
Thanks!

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 52fdb00 (thanks!)

We could fill out the contributing guide a bit more.

@jonatack jonatack merged commit 7c62ebe into bitcoin:master Aug 13, 2024
4 checks passed
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