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

Pre-commit formatting pass on cpp files. #1244

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

nvdbaranec
Copy link
Collaborator

We were behind a bit. Quite a few files got reformatted.

revans2
revans2 previously approved these changes Jun 30, 2023
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

At least it is consistent in how it looks. Really confused by a lot of the formatting choices, but I am not up for a war on any of this.

@ttnghia
Copy link
Collaborator

ttnghia commented Jun 30, 2023

There is no .clang-format config for this repository. Can you provide that file please?

@gerashegalov
Copy link
Collaborator

There is no .clang-format config for this repository. Can you provide that file please?

If pre-commit is used we use cudf's https://github.com/NVIDIA/spark-rapids-jni/blob/branch-23.08/.pre-commit-config.yaml#L10

hyperbolic2346
hyperbolic2346 previously approved these changes Jun 30, 2023
@ttnghia
Copy link
Collaborator

ttnghia commented Jun 30, 2023

That's great, so we can reuse cudf C++ config. And we should do the same for the C++ files in cudf Java folder.

@hyperbolic2346
Copy link
Collaborator

We should automate this or enforce it in CI.

@ttnghia
Copy link
Collaborator

ttnghia commented Jun 30, 2023

I would suggest to create symbolic link to cudf config file so our IDE can find it automatically.

@bdice
Copy link
Contributor

bdice commented Jun 30, 2023

@nvdbaranec FYI, the clang-format version defined in .pre-commit-config.yaml is major version 14, not the same as what cudf uses (16.0.1). I recommend aligning.

@gerashegalov
Copy link
Collaborator

We should automate this or enforce it in CI.

The main purpose of auto-formatting is to remove to the overhead of code-formatting discussions on the PR's. I have been +1 on this. CI does not solve this problem. The pre-commit hook does. Last time it came up, there was no consensus. If it changes we can enforce it by requiring a github action of applying that pre-commit check to succeed without a diff.

@nvdbaranec nvdbaranec dismissed stale reviews from hyperbolic2346 and revans2 via 39cec08 June 30, 2023 18:56
@nvdbaranec
Copy link
Collaborator Author

Updated config version as per @bdice 's suggestion.

@bdice
Copy link
Contributor

bdice commented Jun 30, 2023

Regarding automatic enforcement CI: running pre-commit in CI is how RAPIDS solves this, because it forces local commits and CI builds to adhere to the same rules. See https://github.com/rapidsai/cudf/blob/branch-23.08/ci/check_style.sh for cudf’s script.

@gerashegalov
Copy link
Collaborator

gerashegalov commented Jul 1, 2023

Regarding automatic enforcement CI: running pre-commit in CI is how RAPIDS solves this, because it forces local commits and CI builds to adhere to the same rules. See https://github.com/rapidsai/cudf/blob/branch-23.08/ci/check_style.sh for cudf’s script.

this looks exactly like the CI-based based PR check I described , which could run as a github action

I may have misunderstood @hyperbolic2346 comment assuming it's about a job that periodically re formats and auto-merges reformatted code.

Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Changes look OK other than it needs a signoff commit per contributor guidelines.

@jlowe jlowe mentioned this pull request Jul 3, 2023
@jlowe
Copy link
Member

jlowe commented Jul 5, 2023

build

@nvdbaranec nvdbaranec merged commit 3b3ced7 into NVIDIA:branch-23.08 Jul 5, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants