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 to pre-commit configuration #489

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

musaddiq-ashfaq
Copy link
Contributor

Resolves #488
I have added the code for codespell in the .pre-commit-config.yaml file. Tested the results locally after adding codespell using pre-commit run --all-files and here is the output:

image

image

image

image

It can be seen from the results that codespell have successfully identified the spelling mistakes from the files.

@musaddiq-ashfaq musaddiq-ashfaq requested a review from a team as a code owner January 6, 2025 18:10
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for your interest in helping with this!

Please also fix all the issues reported by the check, by doing some mix of the following:

  • correcting typos
  • adding configuration in pyproject.toml to ignore terms that shouldn't be treated as typos (following the example linked from [ci] add codespell #488)

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@musaddiq-ashfaq
Copy link
Contributor Author

Resolved the issues identified by codespell and also added files to ignore in pyproject.toml

Screenshot from 2025-01-06 23-39-54

Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks! Please see the next round of suggestions.

pyproject.toml Outdated
[tool.codespell]
# note: pre-commit passes explicit lists of files here, which this skip file list doesn't override -
# this is only to allow you to run codespell interactively
skip = "./.git,./build,./python/_skbuild/,.*egg-info.*,./.mypy_cache,./pyproject.toml,./benchmarks/utilities/cxxopts.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skip = "./.git,./build,./python/_skbuild/,.*egg-info.*,./.mypy_cache,./pyproject.toml,./benchmarks/utilities/cxxopts.hpp"
skip = "./.git,./build,./.mypy_cache,./pyproject.toml"

This project doesn't have some of these paths... I'm guessing that maybe you just copied this from another repo. Let's please keep it limited to directory that appear in this project.

pyproject.toml Outdated
@@ -18,6 +18,16 @@ select = [
"B"
]

[tool.codespell]
Copy link
Member

Choose a reason for hiding this comment

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

Please move this tool.codespell table to the head of the file, to preserve alphabetical ordering (by table name). This makes it a little easier to navigate the file as it gets bigger.

pyproject.toml Outdated
skip = "./.git,./build,./python/_skbuild/,.*egg-info.*,./.mypy_cache,./pyproject.toml,./benchmarks/utilities/cxxopts.hpp"
# ignore short words, and typename parameters like OffsetT
ignore-regex = "\\b(.{1,4}|[A-Z]\\w*T)\\b"
ignore-words-list = "thirdparty,couldn"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ignore-words-list = "thirdparty,couldn"

I don't see either thirdparty or couldn in the typos found by codespell... is this necessary? If it's not, please remove it.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change build/test Building and testing the documentation labels Jan 6, 2025
@jameslamb jameslamb changed the title Added codespell to pre-commit configuration Add codespell to pre-commit configuration Jan 6, 2025
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for working through this, looks great! And I'm happy that it caught so many issues.

@jameslamb
Copy link
Member

In the future when you contribute to projects on GitHub, I think you'll find that using a specific-to-one-pull-request branch on your fork, not main, will make a lot of things easier.

As soon as I merge this, all of your commits on this PR will be squashed into a single one on main, and then the history here will be different than main in your fork. If you're unsure about what I'm saying, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

@jameslamb jameslamb merged commit 0a5fdf1 into rapidsai:main Jan 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/test Building and testing the documentation improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ci] add codespell
2 participants