Add C++ code Formatting #420

mabruzzo · 2024-07-18T15:55:17Z

Pull request summary

This PR adds a support for using clang-format with Enzo-E.
I have added extensive documentation.

Detailed Description

This is something we spoken at length about in the past and there is consensus enforcing standard formatting for C/C++ code would be beneficial.

I feel like this is something we could delay indefinitely. So I made 2 particular choices to make this easier to merge:

To avoid merge-conflict I have excluded just about all existing files from clang-format.
- If we don't do this we will be trapped in a cycle of waiting for PRs to be closed (which never ends)
- I think there's value in applying formatting new files and "ripping the band-aid off" later for the older files
I decided to "just make a decision" about code-formatting.
- This is something that we could endlessly bikeshed, and I think it would be beneficial to choose now.
- I basically choose the Google C++ style and made a few very minor adjustments based on a few different choices made in the LLVM style (the two styles are extremely similar). I have put forth a similar style in Grackle.

I have reformatted a small subset of existing files to show what the style looks like. I'm happy to make changes if you see something egregious.
For the most part, I think we should leave iteration on minor details to a later date. That's the beauty of code-formatting.

Relationship with `pre-commit`

The main challenge of using clang-format is that it is not forward/backward compatible (picking an established style should mitigate some of these issues). - In other words, you need to apply a consistent version of clang-format everywhere.

Since clang-format is part of the LLVM project, this can be somewhat non-trivial to enforce (it could be very inconvenient to start requiring people to use).
If we didn't take any steps to address this problem, it could be a large inconvenience.

SOLUTION: To address this challenge, this PR configures the pre-commit software and pre-commit.ci continuous integration service (introduced in #419) to invoke clang-format.

Essentially, you will be able to modify the formatting of proposed changes in a PR without locally installing any software by leaving a comment that says "pre-commit.ci autofix" to a PR (then a commit will be added to your branch to fix the problem).
To easily run clang-format locally, I recommend installing the pre-commit software on your machine and invoking that software. The pre-commit software will download its own personal copy of clang-format to use for formatting (it won't affect the rest of your system)
I have added extensive documentation about how this works (and how to do it).

I implemented a similar solution in Cholla and it works well.

This is a major change or addition (that needs 2 reviewers): unknown

Depends on #419

PR Checklist

New features are documented with narrative docs.

…luded within fortran source code

mabruzzo · 2024-08-15T15:46:24Z

At the last Enzo-E call, it was suggested that I try to format everything and check if it works.

A branch of the repository where everything is formatted can be found here.
I confirmed that everything definitely compiles when we do that

mabruzzo · 2024-11-21T17:23:39Z

pre-commit.ci run

EDIT: For posterity, this comment was used to trigger the new pre-commit.ci continutous integration (this was necessary since I introduced precommit.ci after this PR was created and I didn't want to push a new commit)

mabruzzo · 2024-11-21T17:27:17Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

bwoshea · 2025-01-16T14:48:47Z

src/Enzo/chemistry/EnzoComputeCoolingTime.cpp

Why is this file included in this PR at all? it doesn't seem necessary to do it. Is the goal to demonstrate how the formatting tools work, is it some unrelated cleanup work, or was it included by accident?

(@bwoshea I'm guessing this is one of the "I have reformatted a small subset of existing files" mentioned in the PR description.)

bwoshea · 2025-01-16T14:50:00Z

src/Enzo/chemistry/EnzoComputeCoolingTime.hpp

Same question as for EnzoComputeCoolingTime.cpp - why is this file included in the PR? (Also, to be clear, I don't see anything wrong with the changes in either of these files, I'm just not clear on why they're included.)

bwoshea · 2025-01-16T15:50:12Z

doc/source/devel/style-formatting.rst

+   3. the "pre-commit" `git hook <https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks>__`.
+      This is one of multiple different "hooks" offered by git.
+      The pre-commit software is named after this hook because it was originally designed to be used with this hook.
+      While developers are free to use the pre-commit software with the pre-commit hook, that is **NOT** (and is not discussed on this page)


This is confusing. "that is NOT" what?

this is NOT recommended

(I'll fix that)

Maybe swap the "not recommended" and "recommended" items so the recommended one is first?

This is fixed

I replaced

While developers are free to use the pre-commit software with the pre-commit hook, that is NOT (and is not discussed on this page)"
with
We do NOT generally recommend using the pre-commit software with the pre-commit hook.

@jobordner (I think I effectively addressed your comment here)

As an aside: I don't really care whether people are configuring git's hooks to trigger pre-commit hooks, but I think that is a personal choice after they understand the choice (if people don't understand what they are "signing up for", they will be surprised)

bwoshea · 2025-01-16T15:51:26Z

doc/source/devel/style-formatting.rst

+
+To run the checks locally, we encourage you need to install the pre-commit software.
+This software is written in python and can be installed with ``pip``.
+The `installation instructions <https://pre-commit.com/#installation>`__ also mention an alternative approach where you can run download and run pre-commit without fully installing it (as a "zipapp").


Are there any known conflicts with the requirements for pre-commit and those of other things Enzo-E developers are likely to use, like yt? (I don't know if that's something that has to be explicitly stated unless there's something that is likely to trip up a lot of developers.)

I have no idea. I imagine the answer is "no" since yt also uses pre-commit

doc/source/devel/style-formatting.rst

bwoshea · 2025-01-16T15:53:40Z

doc/source/devel/style-formatting.rst

+
+ * files that are formatted this way will generally have far fewer merge conflicts.
+
+ * Sometimes, you may need to disable clang-format to disable formatting for individual pieces of code.


Can you comment on why somebody might want to disable clang-format?

I think I'm just going to remove that bullet. Off the top of my head, I don't really have a good reason.

I'm sure there are good reasons, but there's no need to invite it

bwoshea · 2025-01-16T15:54:37Z

doc/source/devel/style-formatting.rst

+ * Sometimes, you may need to disable clang-format to disable formatting for individual pieces of code.
+   You can do that with a pair of special comments, `described here <https://releases.llvm.org/18.1.8/tools/clang/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code>`__.
+
+ * Unless you have a very good reason, [#f1]_ **all** newly introduced C/C++ files will be formatted by ``clang-format``.


Maybe add something to the effect of "adherence to the formatting guidelines enforced by clang-format is a requirement for acceptance of pull requests, and deviation from that will require you to explicitly justify why your code is not adhering to that format."

I like this a lot! Here's the replacement text:

Adherence to the formatting guidelines enforced by clang-format is a requirement for acceptance of pull requests, and deviation from that will require you to explicitly justify why your code is not adhering to that format. [#f1]_ We have temporarily disabled clang-format from applying to most older files (listed in .clang-format-ignore), that existed before we adopted clang-format in order to minimize merge conflicts (we plan to enable clang-format for these files in the future).

.. [#f1] A scenario where it might be reasonable to disable clang-format is for a source code file generated by external tools like flex/bison.

bwoshea · 2025-01-16T15:54:57Z

doc/source/devel/style-formatting.rst

+
+ * Unless you have a very good reason, [#f1]_ **all** newly introduced C/C++ files will be formatted by ``clang-format``.
+   With that said, we have temporarily disabled clang-format from applying to most older files (listed in ``.clang-format-ignore``, that existed before we adopted clang-format in order to minimize merge conflicts (we plan to enable ``clang-format`` for these files in the future).
+   We plan to enable formatting on these files in the future.


This line (148) is redundant with the previous sentence.

bwoshea · 2025-01-16T15:57:32Z

@mabruzzo I looked at all of the files in this PR and things look just fine. However (as I commented at the top of a couple of the files) it's not clear to me why the files in Enzo/chemistry are included in this PR. It looks like they've had the new formatting applied to them, so I suspect that they're an example of a formatted version of the code. Can you comment on that? Otherwise, I am happy to approve this PR.

mabruzzo · 2025-01-16T17:04:56Z

Those files are indeed an example of a formatted version of the code.

bwoshea

Approving given that @mabruzzo answered my question about the chemistry code. @jobordner needs to look at this next!

jobordner

Overall looks good, thanks for working on this!

doc/source/devel/style-formatting.rst

jobordner · 2025-02-20T03:51:14Z

src/Enzo/chemistry/EnzoComputeCoolingTime.cpp

(@bwoshea I'm guessing this is one of the "I have reformatted a small subset of existing files" mentioned in the PR description.)

…re building sphinx docs

mabruzzo added 3 commits July 18, 2024 11:14

introducing clang-format.

6d52578

apply clang-format to all files in src/Enzo/chemistry

cacbf41

add full documentation

0428b7f

mabruzzo mentioned this pull request Aug 8, 2024

Code formatting grackle-project/grackle#222

Open

mabruzzo added 2 commits August 15, 2024 11:29

Merge branch 'main' into add-cpp-formatting

f90ea96

identify the file(s) that shouldn't be formatted because they are inc…

9ce55c3

…luded within fortran source code

jobordner self-assigned this Oct 17, 2024

gregbryan requested review from bwoshea and jobordner October 17, 2024 16:33

Merge branch 'main' into add-cpp-formatting

deeae54

[pre-commit.ci] auto fixes from pre-commit.com hooks

9f62343

for more information, see https://pre-commit.ci

bwoshea reviewed Jan 16, 2025

View reviewed changes

doc/source/devel/style-formatting.rst Outdated Show resolved Hide resolved

bwoshea reviewed Jan 16, 2025

View reviewed changes

bwoshea approved these changes Jan 16, 2025

View reviewed changes

jobordner approved these changes Feb 20, 2025

View reviewed changes

mabruzzo added 4 commits February 20, 2025 11:01

Fixing up documentation based on PR review

4a41481

another set of tweaks to the docs

33d77ac

ReadTheDocs has broken builds unless you explicitly specify that we a…

9f8ce17

…re building sphinx docs

Merge branch 'fixing-readthedocs' into add-cpp-formatting

802a56c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C++ code Formatting #420

Add C++ code Formatting #420

mabruzzo commented Jul 18, 2024 •

edited

Loading

mabruzzo commented Aug 15, 2024

mabruzzo commented Nov 21, 2024 •

edited

Loading

mabruzzo commented Nov 21, 2024

bwoshea Jan 16, 2025

jobordner Feb 20, 2025

bwoshea Jan 16, 2025

bwoshea Jan 16, 2025

mabruzzo Jan 16, 2025

jobordner Feb 20, 2025

mabruzzo Feb 20, 2025

bwoshea Jan 16, 2025

mabruzzo Jan 16, 2025

bwoshea Jan 16, 2025

mabruzzo Feb 20, 2025 •

edited

Loading

bwoshea Jan 16, 2025

mabruzzo Feb 20, 2025

bwoshea Jan 16, 2025

mabruzzo Feb 20, 2025

bwoshea commented Jan 16, 2025

mabruzzo commented Jan 16, 2025

bwoshea left a comment

jobordner left a comment

jobordner Feb 20, 2025


		* files that are formatted this way will generally have far fewer merge conflicts.

		* Sometimes, you may need to disable clang-format to disable formatting for individual pieces of code.

Add C++ code Formatting #420

Are you sure you want to change the base?

Add C++ code Formatting #420

Conversation

mabruzzo commented Jul 18, 2024 • edited Loading

Pull request summary

Detailed Description

Relationship with pre-commit

PR Checklist

mabruzzo commented Aug 15, 2024

mabruzzo commented Nov 21, 2024 • edited Loading

mabruzzo commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabruzzo Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwoshea commented Jan 16, 2025

mabruzzo commented Jan 16, 2025

bwoshea left a comment

Choose a reason for hiding this comment

jobordner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabruzzo commented Jul 18, 2024 •

edited

Loading

Relationship with `pre-commit`

mabruzzo commented Nov 21, 2024 •

edited

Loading

mabruzzo Feb 20, 2025 •

edited

Loading