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

Adding spell checker as a pre-commit hook #1544

Merged
merged 4 commits into from
May 21, 2021

Conversation

Skaft
Copy link
Contributor

@Skaft Skaft commented May 19, 2021

Changelog / Overview

  • Adds a pre-commit hook for codespell
  • Fixes a bunch of spelling mistakes in code and documentation

Motivation

Removes existing spelling mistakes, and prevents future ones.

Explanation for Changes

Spelling errors will keep cropping up, with a pre-commit hook to hunt them down we can reduce the problem. Of course codespell is not the only option for that, and there might be better alternatives. This is the first I tried, but it seems to work well out of the box.

Testing Status

Most changes are in documentation, but some variable names have been changed. So I ran the pytest suite which passed.

Further Comments

  • Possibly controversial: There are a few cases where it will treat truncated variable names as spelling errors. I have changed the ones it complained about, so you can see the extent of that meddling within this PR. In my opinion, they are either variable names that benefit from a full name anyway, or names that can be changed without consequence. But maybe this could be an annoyance at some later point.
  • I'm not sure the dependency setup is complete. Should I tweak the poetry files in some way?
  • The errors fixed here are all it could find in the current state of the repo, except for a false positive in a .svg file. Lines triggering false positives can be listed in .codespell_ignorelines.
  • Codespell works by looking for common spelling mistakes, and there are different dictionaries it can use to find those. It's currently running on defaults, but tweaking that setup might improve results.

Checklist

  • I have read the Contributing Guidelines
  • I have written a descriptive PR title (see top of PR template for examples)
  • I have written a changelog entry for the PR or deem it unnecessary
  • My new functions/classes either have a docstring or are private
  • My new functions/classes have tests added and (optional) examples in the docs
  • My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

  • The PR title is descriptive enough
  • The PR is labeled correctly
  • The changelog entry is completed if necessary
  • Newly added functions/classes either have a docstring or are private
  • Newly added functions/classes have tests added and (optional) examples in the docs
  • Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

@Skaft
Copy link
Contributor Author

Skaft commented May 19, 2021

Ah-hmm. Right. I guess I can't just ignore the changelog. Should I correct the spelling errors in it, or set up codespell to ignore that folder? I'll make it skip svg files, those aren't texty enough I suppose.

@Skaft
Copy link
Contributor Author

Skaft commented May 19, 2021

Tried to add the .codespellrc and use it to make it skip the changelog directory and svg files. Seems to work locally, not sure why it's different on CI?

@Skaft Skaft marked this pull request as draft May 20, 2021 04:04
@naveen521kk
Copy link
Member

Should I correct the spelling errors in it, or set up codespell to ignore that folder?

You can correct spelling in the changelog, no problems with that :).

@Skaft
Copy link
Contributor Author

Skaft commented May 21, 2021

You can correct spelling in the changelog, no problems with that :).

Thanks for input ^_^

  • Looks like this is passing CI now! I've been having some issues with line endings. Not sure how much of that struggle is my own fault, and how much is on the shoulders of codespell / pre-commit. This currently unmerged PR could be worth keeping an eye on though in case these problems resurface.
  • I've added the file .codespell_ignorelines where any lines triggering false positives can be chucked. There's currently a line from a .svg file in it, but that was the only troublesome line so far.

I'll open this for review now as I think it's working properly.

@Skaft Skaft marked this pull request as ready for review May 21, 2021 08:55
Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@naveen521kk naveen521kk merged commit 60a7757 into ManimCommunity:master May 21, 2021
@naveen521kk naveen521kk added the maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements label May 21, 2021
@Skaft Skaft deleted the spelling branch May 21, 2021 13:33
@peternewman
Copy link

peternewman commented Sep 7, 2022

  • Looks like this is passing CI now! I've been having some issues with line endings. Not sure how much of that struggle is my own fault, and how much is on the shoulders of codespell / pre-commit. This currently unmerged PR could be worth keeping an eye on though in case these problems resurface.

Was it just the line endings of the ignore file you were having issues with then @Skaft ?

@Skaft
Copy link
Contributor Author

Skaft commented Oct 1, 2022

@peternewman Sorry, I don't recall the specifics, it's been over a year since I looked at this.

@peternewman
Copy link

Heh, no worries @Skaft .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants