-
Notifications
You must be signed in to change notification settings - Fork 304
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
Use pre-commit for CI style checks. #3062
Conversation
@@ -1,4 +1,4 @@ | |||
# Copyright (c) 2020-2022, NVIDIA CORPORATION. | |||
# Copyright (c) 2019-2022, NVIDIA CORPORATION. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was updated to better support pre-commit
(I copied these changes from cudf). It also supports autofixing now, so if the style checks fail, it will tell you exactly which files need to change and what they should say.
I'm going to go ahead and revert the two commits for clang-format and flake8 Cython that make this diff large. I'll make separate PRs for those after the first commit is merged. |
args: ["--config=setup.cfg"] | ||
files: python/.*$ | ||
types: [file] | ||
types_or: [python] # TODO: Enable [python, cython] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will enable flake8 on Cython files in a follow-up PR. Commit 2e4f34f is what I'll put in that PR.
exclude: | | ||
(?x)^( | ||
cpp/libcugraph_etl| | ||
cpp/tests/c_api/.* | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These exclusions are needed because run-clang-format.py
was not kept in sync with the repo's contents. Using pre-commit solves this problem. I don't want a large diff in this PR so I will enable clang-format on those files in a follow-up PR. Commit 9825b90 is what I'll put in that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportBase: 58.86% // Head: 58.95% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-23.02 #3062 +/- ##
================================================
+ Coverage 58.86% 58.95% +0.08%
================================================
Files 131 131
Lines 7848 7857 +9
================================================
+ Hits 4620 4632 +12
+ Misses 3228 3225 -3
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. |
rerun tests |
@gpucibot merge |
#3062 added pre-commit hooks for formatting C and C++ code. However, libcugraph_etl and the C API unit tests are currently excluded from that check. The intention was to get back around to them and include them in the testing. However, that slipped through the cracks. This PR adds the pre-commit hooks for those files as well. Authors: - Chuck Hastings (https://github.com/ChuckHastings) Approvers: - Bradley Dice (https://github.com/bdice) - Seunghwa Kang (https://github.com/seunghwak) - Mark Harris (https://github.com/harrism) URL: #4332
This PR adopts
pre-commit
in CI style checks and updates a few of the hooks (flake8, clang-format, and the copyright checker).This helps with the ongoing transition to GitHub Actions by centralizing style checks.
I also applied a significant set of changes from
clang-format
andflake8
suggestions in commits cfe6395 and adf0741. To minimize the diff in this PR, we can split those two commits into their own PRs with some small tweaks to thepre-commit
configuration. Let me know if you'd like me to do this.