Skip to content

Conversation

@JackPGreen
Copy link
Contributor

@JackPGreen JackPGreen commented Oct 23, 2025

In #946 the codebase was reformatted for consistency, but as this was not enforced the codebase has drifted since then.

To avoid this happening again, enforce this at the PR stage.

Testing:

Post-merge:

@JackPGreen JackPGreen self-assigned this Nov 25, 2025
@JackPGreen JackPGreen marked this pull request as ready for review November 25, 2025 12:54
Copy link
Collaborator

@ihsandemir ihsandemir left a comment

Choose a reason for hiding this comment

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

In the example runs shared, the printed error message is not informative enough. It says

./hazelcast/test/src/sql_test.cpp:1795:33: error: code should be clang-formatted [-Wclang-format-violations]
    auto statement = [&result](){
                                ^

But what is the actual problem with that line? Is there a way also print details?

Copy link
Collaborator

@ihsandemir ihsandemir left a comment

Choose a reason for hiding this comment

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

the shared example success actually has a failed step code coverage?

@JackPGreen
Copy link
Contributor Author

the shared example success actually has a failed step code coverage?

The code coverage is already failing (or at least, flakey) - https://github.com/hazelcast/hazelcast-cpp-client/actions/runs/19500936469/job/55814670258

@JackPGreen
Copy link
Contributor Author

In the example runs shared, the printed error message is not informative enough. It says

./hazelcast/test/src/sql_test.cpp:1795:33: error: code should be clang-formatted [-Wclang-format-violations]
    auto statement = [&result](){
                                ^

But what is the actual problem with that line? Is there a way also print details?

This is all the information the tool offers. It's just a wrapper around clang-format, (re)formatting the input and then checking the output is the same - if not (i.e. the code was not already formatted) then it raises an error.

It does raise a summary that shows what files have formatting errors.

We already tell contributors to format the code - this is just a test to assert they have:

- Format the code using `clang-format`
- [`scripts/format-all.sh`](../scripts/format-all.sh) can do this for you

ref: ${{ needs.get-refs.outputs.ref }}
token: ${{ secrets.GH_TOKEN }}

- uses: jidicula/clang-format-action@6cd220de46c89139a0365edae93eee8eb30ca8fe # v4.16.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this action a secure reliable code base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this action a secure reliable code base?

I think so, and it's used by many others.

@ihsandemir
Copy link
Collaborator

In the example runs shared, the printed error message is not informative enough. It says

./hazelcast/test/src/sql_test.cpp:1795:33: error: code should be clang-formatted [-Wclang-format-violations]
    auto statement = [&result](){
                                ^

But what is the actual problem with that line? Is there a way also print details?

This is all the information the tool offers. It's just a wrapper around clang-format, (re)formatting the input and then checking the output is the same - if not (i.e. the code was not already formatted) then it raises an error.

It does raise a summary that shows what files have formatting errors.

We already tell contributors to format the code - this is just a test to assert they have:

- Format the code using `clang-format`
- [`scripts/format-all.sh`](../scripts/format-all.sh) can do this for you

Did you consider using:
git clang-format --diff master

This seems to show better about what to be changed and it works on only the changed files compared to master.

@JackPGreen
Copy link
Contributor Author

In the example runs shared, the printed error message is not informative enough. It says

./hazelcast/test/src/sql_test.cpp:1795:33: error: code should be clang-formatted [-Wclang-format-violations]
    auto statement = [&result](){
                                ^

But what is the actual problem with that line? Is there a way also print details?

This is all the information the tool offers. It's just a wrapper around clang-format, (re)formatting the input and then checking the output is the same - if not (i.e. the code was not already formatted) then it raises an error.
It does raise a summary that shows what files have formatting errors.
We already tell contributors to format the code - this is just a test to assert they have:

- Format the code using `clang-format`
- [`scripts/format-all.sh`](../scripts/format-all.sh) can do this for you

Did you consider using: git clang-format --diff master

This seems to show better about what to be changed and it works on only the changed files compared to master.

There are a couple with this approach (although yes, the output is nicer):

  • we run the PR builder both on pull_request and also workflow_dispatch - when triggered manually, it's difficult to figure out what base to diff against
  • I can't find an off-the-shelf action, we'd need to implement our own

I think the net result of both approaches is the same.

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.

2 participants