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 codespell support (config, workflow to detect/not fix) and make it fix few typos #2296

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

yarikoptic
Copy link
Contributor

More about codespell: https://github.com/codespell-project/codespell .

I personally introduced it to dozens if not hundreds of projects already and so far only positive feedback.

CI workflow has 'permissions' set only to 'read' so also should be safe.

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w -i 3 -C 2 ./pkg/common/git/git.go",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@yarikoptic yarikoptic requested a review from a team as a code owner April 16, 2024 13:37
@wolfogre
Copy link
Member

Hi Yaroslav, I remember I had refused your PR like this to Gitea because it touched too many files (50+) involving docs, js, go files and template files, making it almost impossible to review. Sorry again if it made you unhappy.

However, for this PR, I don't see any reason not to go, all changes are all reasonable and reviewable. 👍

wolfogre
wolfogre previously approved these changes Apr 17, 2024
Copy link
Member

Choose a reason for hiding this comment

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

I am sure if we should touch index.js since I remember it's copied from actions/runner.

@ChristopherHX I think you should take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I would prefer to exclude that pkg/runner/hashfiles/index.js file, it's an 1:1 copy.
actions/tookit still has that typos, should be fixed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, excluded

here is the force pushed diff
❯ git diff gh-yarikoptic/enh-codespell..
diff --git a/.codespellrc b/.codespellrc
index fe7ef1c..4857dfe 100644
--- a/.codespellrc
+++ b/.codespellrc
@@ -1,6 +1,6 @@
 [codespell]
 # Ref: https://github.com/codespell-project/codespell#using-a-config-file
-skip = .git*,go.sum,package-lock.json,*.min.*,.codespellrc,testdata
+skip = .git*,go.sum,package-lock.json,*.min.*,.codespellrc,testdata,./pkg/runner/hashfiles/index.js
 check-hidden = true
 ignore-regex = .*Te\{0\}st.*
 # ignore-words-list =
diff --git a/pkg/runner/hashfiles/index.js b/pkg/runner/hashfiles/index.js
index 37e21db..f2bed37 100644
--- a/pkg/runner/hashfiles/index.js
+++ b/pkg/runner/hashfiles/index.js
@@ -942,10 +942,10 @@ class Summary {
         return this.addRaw(element).addEOL();
     }
     /**
-     * Adds a collapsible HTML details element to the summary buffer
+     * Adds a collapsable HTML details element to the summary buffer
      *
      * @param {string} label text for the closed state
-     * @param {string} content collapsible content
+     * @param {string} content collapsable content
      *
      * @returns {Summary} summary instance
      */
@@ -1844,7 +1844,7 @@ class Pattern {
             // false for `/foo` but returns true for `/foo/`. Append a trailing slash to handle that quirk.
             if (!itemPath.endsWith(path.sep)) {
                 // Note, this is safe because the constructor ensures the pattern has an absolute root.
-                // For example, formats like C: and C:foo on Windows are resolved to an absolute root.
+                // For example, formats like C: and C:foo on Windows are resolved to an aboslute root.
                 itemPath = `${itemPath}${path.sep}`;
             }
         }

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.55%. Comparing base (5a80a04) to head (57abb52).
Report is 50 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2296       +/-   ##
===========================================
+ Coverage   61.56%   76.55%   +14.98%     
===========================================
  Files          53       59        +6     
  Lines        9002     7506     -1496     
===========================================
+ Hits         5542     5746      +204     
+ Misses       3020     1238     -1782     
- Partials      440      522       +82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yarikoptic
Copy link
Contributor Author

Thank you @wolfogre ! FWIW -- we can exclude any file you like from being checked/fixed by codespell.

my 1c: Number of files with typos is not an indicator of deficiency in the goal of having typo free code base ;-) typos are usually easy to spot and relatively easy to review IMHO since it is quite often a typo or two in a file. GitHub also provides means to mark reviewed files so they collapse. So it becomes a matter of a few quality minutes of time to review and then live with (much more so) typo free codebase . But indeed a number of files quite often scary people away, but I can find some brave projects which made the dive and so far I have not heard much of complaints.

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@mergify mergify bot merged commit 843cd94 into nektos:master Apr 19, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants