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

Integrate clang-format #176

Merged
merged 4 commits into from
Jul 6, 2024
Merged

Integrate clang-format #176

merged 4 commits into from
Jul 6, 2024

Conversation

lhearachel
Copy link
Collaborator

@lhearachel lhearachel commented Apr 4, 2024

Usage

For convenience, a format.sh script is included; the following command will format the full source tree according to rules specified in .clang-format:

./format.sh

A pre-commit git hook is also included, along with instructions for contributors on how to integrate it into their commit workflow.

Next Steps

  • clang-format does not support renaming constants, variables, etc. We will need a separate clang-tidy integration if we want to standardize that process.

@lhearachel lhearachel changed the title Clang format Integrate clang-format Apr 4, 2024
@lhearachel
Copy link
Collaborator Author

This PR is nonessential, and functionally on-hold; I'll revisit it here and there as I have time to do so.

@mid-kid
Copy link
Member

mid-kid commented Apr 17, 2024

Spotted this in my notifications. I don't have much to comment on regarding the style, only the implementation:

  • I think it's awkward to invoke the build system for this, as it's not building anything, rather modifying the build tree. It's better as a maintenance script, not something to run as part of any build.
  • Regarding auto-formatting on PR, I'd rather it give a CI warning/error (preferably warning) if the autoformatter detects formatting changes in the changed lines. This allows for the possibility to evaluate the formatting changes and possibly choose to override the formatter's decision. Kind of like linux' checkpatch.pl

@lhearachel
Copy link
Collaborator Author

I think it's awkward to invoke the build system for this, as it's not building anything, rather modifying the build tree. It's better as a maintenance script, not something to run as part of any build.

I don't disagree, but Meson gives it to us for free out of the box, and it saves us having to write and maintain a script to traverse/walk our source tree. Maybe it should just be a separate ./format.sh script that serves to invoke the ninja target?

Regarding auto-formatting on PR, I'd rather it give a CI warning/error (preferably warning) if the autoformatter detects formatting changes in the changed lines. This allows for the possibility to evaluate the formatting changes and possibly choose to override the formatter's decision. Kind of like linux' checkpatch.pl

What about a git hook?

@mid-kid
Copy link
Member

mid-kid commented Apr 19, 2024

Meson gives it to us for free out of the box

Oh, I didn't know this. Quite a questionable feature, but if it needs no extra work then I don't mind.

What about a git hook?

I'm not sure, I've never used a git hook in a multiperson project, but I think this'd be most useful to newbie contributors, so it should be default and hard to overlook.

@lhearachel lhearachel marked this pull request as draft June 23, 2024 06:50
@lhearachel lhearachel force-pushed the clang-format branch 2 times, most recently from fd03660 to 41929c8 Compare June 23, 2024 07:53
@lhearachel lhearachel marked this pull request as ready for review June 23, 2024 08:04
@lhearachel lhearachel force-pushed the clang-format branch 2 times, most recently from 33a88ef to 1a9c930 Compare July 6, 2024 21:39
@lhearachel lhearachel force-pushed the clang-format branch 5 times, most recently from 8714748 to 9b75a12 Compare July 6, 2024 22:29
@lhearachel lhearachel merged commit 905cdd3 into pret:main Jul 6, 2024
2 checks passed
github-actions bot pushed a commit that referenced this pull request Jul 6, 2024
@lhearachel lhearachel deleted the clang-format branch October 13, 2024 09:05
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