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 github action for C++ clang-format style check #1597

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

yinqingh
Copy link
Collaborator

Fix #202.

This PR includes two changes:

  • Reformat all files based on cudf .clang-format style
  • Add github action for C++ clang-format style check

@pxLi
Copy link
Collaborator

pxLi commented Nov 28, 2023

Please help provide some test runs (like in your forked repos), to show both success and failure cases, thanks

@pxLi pxLi added the build label Nov 28, 2023
@pxLi
Copy link
Collaborator

pxLi commented Nov 28, 2023

also cc @ttnghia to help check if the auto-formatted changes look good thanks

@pxLi pxLi added the test label Nov 28, 2023
ttnghia
ttnghia previously approved these changes Nov 28, 2023
@pxLi
Copy link
Collaborator

pxLi commented Nov 28, 2023

seems we should have enough time to merge this into 23.12 to avoid auto-merge commits to override the format checks.

re-targeted it to branch-23.12

@pxLi pxLi changed the base branch from branch-24.02 to branch-23.12 November 28, 2023 04:25
@pxLi pxLi dismissed ttnghia’s stale review November 28, 2023 04:25

The base branch was changed.

name: clang format check

on:
pull_request_target:
Copy link
Collaborator

@pxLi pxLi Nov 28, 2023

Choose a reason for hiding this comment

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

just saw you were not getting the correct merged commit sha with pull_request_target + pre-commit/action.
then try using pull_request event directly.

some other options could be that keep pull_request_target trigger, but then checkout required merge sha (this one could help avoid people touching workflow file directly to try pass CI unexpectedly)

    with:
      ref: "${{ github.event.pull_request.merge_commit_sha }}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Seems it always checkout the default branch in the tests. Will fix this. Thanks!

name: clang format check

on:
pull_request:
Copy link
Collaborator Author

@yinqingh yinqingh Nov 28, 2023

Choose a reason for hiding this comment

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

Changed to pull_request event

@pxLi
Copy link
Collaborator

pxLi commented Nov 28, 2023

just caught a new merged commit format issue at https://github.com/NVIDIA/spark-rapids-jni/actions/runs/7014779963/job/19083019630?pr=1597 👍

please upmerge and re-format to get a clean pass, we should be good to merge

@pxLi
Copy link
Collaborator

pxLi commented Nov 28, 2023

build

@pxLi pxLi merged commit b396329 into NVIDIA:branch-23.12 Nov 28, 2023
3 checks passed
@yinqingh yinqingh deleted the clang-format branch November 28, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add formatting tools and enforcement of C++ style
3 participants