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 as a linter #12097

Merged
merged 17 commits into from
Nov 22, 2022
Merged

Add codespell as a linter #12097

merged 17 commits into from
Nov 22, 2022

Conversation

benfred
Copy link
Member

@benfred benfred commented Nov 9, 2022

Description

This adds codespell as a linter to the pre-commit config, and fixes various spelling errors it identifies (https://github.com/codespell-project/codespell)

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

This adds codespell as a linter to the pre-commit config, and fixes various
spelling errors it identifies  (https://github.com/codespell-project/codespell)
setup.cfg Outdated

[codespell]
skip = ./.git,./.github,./cpp/build,.*egg-info.*,versioneer.py,./.mypy_cache,./cpp/tests,./python/cudf/cudf/tests
ignore-words-list = inout,indext,offsett,dout,unparseable,incase,afterall,fiter,trings
Copy link
Contributor

Choose a reason for hiding this comment

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

This list is not that long! 💯

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm cheating a bit and ignoring the tests =)

(The tests have some test data in there that was messing it up - but wasn't really typos)

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 that’s reasonable. We can always expand later if we wish. The errors fixed in this PR are worthwhile even if not covering the whole repo.

Copy link
Contributor

@davidwendt davidwendt Nov 9, 2022

Choose a reason for hiding this comment

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

You probably could add ./java/src/test to this list and then remove trings
And add ./cpp/include/cudf_test/cxxopts.hpp and then remove fiter.
I'm not finding indext or afterall in the code.
Is offsett in here because of offsetted? Maybe those can be corrected.

Copy link
Contributor

@davidwendt davidwendt Nov 9, 2022

Choose a reason for hiding this comment

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

Also, should the skip list include ./python/cudf/_skbuild and ./python/strings_udf/_skbuild as well?
How about __pycache__?
Is there any way this could use the .gitignore files so we do not have to maintain both?

Copy link
Contributor

@bdice bdice Nov 10, 2022

Choose a reason for hiding this comment

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

I believe cxxopts.hpp is vendored and that file should be ignored rather than edited. I believe that specific name fiter is supposed to mean “format iterator” and is not a typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it should not be linted at all.
If you look at the history for cxxopts.hpp it has been modified it several times.
I realize fiter is not a typo but it seems odd to create an exception for it for the entire repo.
Either we should not lint this file or we should fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with David that we should not lint this file. (My previous comment may have been unclear.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the cxxopts.hpp from linting - and applied @wence- suggestion about the ignore regex to exclude template params types. The ignore-words list is down to just inout,unparseable now, and I think those are both valid words myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect! That's ideal.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Nice @benfred! I'd support including this hook.

Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Love this code spell linter addition ❤️

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 9, 2022
@davidwendt
Copy link
Contributor

ok to test

@benfred benfred requested review from a team as code owners November 9, 2022 19:21
@benfred benfred requested review from vyasr, isVoid and ttnghia November 9, 2022 19:21
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added CMake CMake build issue Java Affects Java cuDF API. Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Nov 9, 2022
@davidwendt davidwendt requested a review from vuule November 9, 2022 19:33
@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.02@b743f2f). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-23.02   #12097   +/-   ##
===============================================
  Coverage                ?   88.24%           
===============================================
  Files                   ?      137           
  Lines                   ?    22586           
  Branches                ?        0           
===============================================
  Hits                    ?    19931           
  Misses                  ?     2655           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Yes, thanks!

* use ignore-regex to exclude template params like OffsetT
* slim down list of ignore-words to just inout/unparseable
* exclude ./cpp/include/cudf_test/cxxopts.hpp
@bdice bdice changed the base branch from branch-22.12 to branch-23.02 November 18, 2022 13:32
@bdice
Copy link
Contributor

bdice commented Nov 18, 2022

@gpucibot merge

@benfred
Copy link
Member Author

benfred commented Nov 18, 2022

rerun tests

@bdice bdice changed the base branch from branch-23.02 to branch-22.12 November 21, 2022 20:26
@bdice bdice requested review from a team as code owners November 21, 2022 20:26
@bdice bdice changed the base branch from branch-22.12 to branch-23.02 November 21, 2022 20:26
@bdice
Copy link
Contributor

bdice commented Nov 21, 2022

I changed (and reset) the target branch so that the Branch Checker CI would be unblocked. It looks like CI will need to re-run, then this can be merged.

@bdice bdice removed request for a team November 21, 2022 20:27
@bdice
Copy link
Contributor

bdice commented Nov 21, 2022

@gpucibot merge

@galipremsagar
Copy link
Contributor

rerun tests

@ttnghia
Copy link
Contributor

ttnghia commented Nov 22, 2022

Fix typo in ChunkedReaderJni.cpp

OMG I can't know why I've made that typo. I swear that I stared at that comment many times before merging 😂

@rapids-bot rapids-bot bot merged commit d49e412 into rapidsai:branch-23.02 Nov 22, 2022
@benfred benfred deleted the codespell branch November 22, 2022 05:25
benfred added a commit to benfred/raft that referenced this pull request Dec 8, 2022
Exclude the changelog from the pre-commit changelog - this is to prevent
CI failures after release when the changelog is updated, as suggested
here rapidsai/cudf#12097 (review)

This also exports the `__version__` for pylibraft / raft_dask, which was missing
previously
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Dec 8, 2022
Exclude the changelog from the pre-commit codespell check- this is to prevent CI failures after release when the changelog is updated, as suggested here rapidsai/cudf#12097 (review)

This also exports the `__version__` for pylibraft / raft_dask, which was missing previously

Authors:
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #1083
benfred added a commit to benfred/cuml that referenced this pull request Mar 9, 2023
Similar to rapidsai/cudf#12097, this adds codespell
as a linter to the pre-commit config, and fixes various spelling errors
it highlights. (https://github.com/codespell-project/codespell)
benfred added a commit to benfred/cuml that referenced this pull request Mar 9, 2023
Similar to rapidsai/cudf#12097, this adds codespell
as a linter to the pre-commit config, and fixes various spelling errors
it highlights. (https://github.com/codespell-project/codespell)
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Mar 14, 2023
Similar to rapidsai/cudf#12097, this adds codespell as a linter to the pre-commit config, and fixes various spelling errors it highlights. (https://github.com/codespell-project/codespell)

Authors:
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Carl Simon Adorf (https://github.com/csadorf)
  - William Hicks (https://github.com/wphicks)
  - Joseph (https://github.com/jolorunyomi)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #5265
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Mar 14, 2023
Following the example of rapidsai/cudf#12097, this PR adds [codespell](https://github.com/codespell-project/codespell) as a linter for rmm.

Note: I have not included a section in the CONTRIBUTING.md about how to use this (as was done in cudf's PR) because I plan to overhaul the contributing guides for all RAPIDS repos in the near term, and have a single source in docs.rapids.ai with common information about linters used in RAPIDS.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Rong Ou (https://github.com/rongou)
  - Ben Frederickson (https://github.com/benfred)
  - Mark Harris (https://github.com/harrism)

URL: #1231
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Mar 14, 2023
Following the example of rapidsai/cudf#12097, this PR adds [codespell](https://github.com/codespell-project/codespell) as a linter for cuspatial.

Note: I have not included a section in the CONTRIBUTING.md about how to use this (as was done in cudf's PR) because I plan to overhaul the contributing guides for all RAPIDS repos in the near term, and have a single source in docs.rapids.ai with common information about linters used in RAPIDS.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Michael Wang (https://github.com/isVoid)
  - Ray Douglass (https://github.com/raydouglass)

URL: #992
trxcllnt pushed a commit to trxcllnt/cuspatial that referenced this pull request Mar 15, 2023
Following the example of rapidsai/cudf#12097, this PR adds [codespell](https://github.com/codespell-project/codespell) as a linter for cuspatial.

Note: I have not included a section in the CONTRIBUTING.md about how to use this (as was done in cudf's PR) because I plan to overhaul the contributing guides for all RAPIDS repos in the near term, and have a single source in docs.rapids.ai with common information about linters used in RAPIDS.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Michael Wang (https://github.com/isVoid)
  - Ray Douglass (https://github.com/raydouglass)

URL: rapidsai#992
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants