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 job to GitHub Actions (#571) #573

Merged
merged 10 commits into from
Mar 31, 2023

Conversation

KyleFromKitware
Copy link
Collaborator

@KyleFromKitware KyleFromKitware commented Mar 29, 2023

Addresses #571

@KyleFromKitware KyleFromKitware marked this pull request as ready for review March 29, 2023 15:56
@KyleFromKitware KyleFromKitware marked this pull request as draft March 29, 2023 15:57
@KyleFromKitware KyleFromKitware force-pushed the codespell branch 9 times, most recently from e173735 to 8486aad Compare March 29, 2023 18:33
.codespellrc Outdated
# Disable warnings about binary files
quiet-level = 2
skip = */.git,*/common_tools/cloc
ignore-words-list = thur,inout,te,nd,lod,aci,nin,nnumber,wile,reall,somwhere
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gsjaardema In commit e8a48b3, you restored a number of email addresses to somwhere.com. Could you please explain the motivation for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I had changed them in a previous commit and then decided that this correction wasn't necessarily a spelling error since maybe somwhere.com is as valid as somewhere.com as an email address/domain name. Similar to not changing bogous to bogus -- maybe was intentionally not a valid word...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It may be a valid domain name, but I think the intention was clearly somewhere.com. I think we should fix it. @bartlettroscoe WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Where is somwhere flagged? I don't think that is valid name.

Copy link
Member

Choose a reason for hiding this comment

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

This is fixed in commit d898491 (#573).

@KyleFromKitware KyleFromKitware marked this pull request as ready for review March 29, 2023 19:15
@KyleFromKitware KyleFromKitware changed the title WIP: Add Codespell job to GitHub Actions Add Codespell job to GitHub Actions Mar 29, 2023
Copy link
Member

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

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

I just want to make sure this is set up so it can be run locally as well. I would really like to have a TriBITS ctest test that runs the spell checking as part of the test suite (if codespell is available).

Also, it would be great to put in a commit hook that runs codespell on files modified in a commit (if codespell is installed locally).

@bartlettroscoe bartlettroscoe added type: enhancement component: testing Dealing with automated testing not specific to any other component labels Mar 30, 2023
@bartlettroscoe bartlettroscoe changed the title Add Codespell job to GitHub Actions Add Codespell job to GitHub Actions (#571) Mar 30, 2023
…BITSPub#571)

While running codespell 2.2.1, it flagged 'n{nothing}Does' in:

  #print("\n{nothing}Does ...")

which is not a misspelling (and a later version of codespell does not flag
this) but I wanted to remove it since it was not really being used.

And while I was at it, I remove all of the commented out print() statements
(since those were just there for debugging).
These really should be deleted since we don't maintain bare clones anymore,
and if we did, we could not care about these hooks.

I did this as part of TriBITSPub#571.
I also added a bash script to automate installing the git hook scripts with
printed messages.
Copy link
Member

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

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

All of the changes look good to me added by @KyleFromKitware. I have added a local CTest test TriBITS_codespell that is defined and runs locally when the 'codespell' command is present. Also, I added a Git commit-msg hook that runs codespell on the commit message string itself.

We can still add a Git pre-commit hook that runs codespell before every commit but I think that may be overkill since there is a local CTest test that runs it. Also, I have found that codespell missing a lot of misspelling so having the codespell test fail locally is a good reminder to go back into the modified files and look for any other misspelling using flyspell under emacs.

@bartlettroscoe bartlettroscoe merged commit 2da746a into TriBITSPub:master Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: testing Dealing with automated testing not specific to any other component type: enhancement
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants