-
Notifications
You must be signed in to change notification settings - Fork 51
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
clang-format #1032
clang-format #1032
Conversation
4bbf790
to
79dafdd
Compare
a789bf8
to
5f89c09
Compare
c00dd15
to
c239eea
Compare
I think we can use pre-commit.ci to run this on PRs and automatically let it push a "fix" on a PR for style: Seen in PRs to |
619e346
to
b7ebdef
Compare
f2cec0f
to
7229cbf
Compare
I might have gotten this to work |
.pre-commit-config.yaml
Outdated
@@ -1,7 +1,11 @@ | |||
# See https://pre-commit.com for more information | |||
# See https://pre-commit.com/hooks.html for more hooks | |||
repos: | |||
- repo: https://github.com/franzpoeschel/openPMD-api-pre-commit-hooks | |||
rev: aa38b2ad4fae42db05c19de211f43df6a7981bc5 | |||
- repo: local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use https://github.com/pre-commit/mirrors-clang-format for this :)
Alternatively, if we want to use our own scripts and env, let's move them in a sub-directory in .github/workflows/clang-format/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the current solution actually because it gives us a script that users can also call "bare-metal" if they want, without having to set up pre-commit. Also, it's a 5-line script that saves us from relying on an external dependency.
I can move it to the workflows thing, sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jup, I think that's a good point and it integrates well.
c92c6b0
to
79ca528
Compare
Would you think writing a small Python script that helps us migrate old pull requests past a reformatting of the whole code base is worth it? I think it should be possible. EDIT: I needed this somewhere else too, so I quickly hacked something up https://github.com/franzpoeschel/reformat-rebase/blob/master/reformat-rebase.py. Needs some documentation to get usable, but we don't need to wait on my PRs to merge the clang-format thing. I can also help rebasing other PRs past the reformat if anyone wants. |
2322590
to
92243da
Compare
92243da
to
c7dbf24
Compare
I think I've found a solution/workaround for the
|
1) Change OPENPMD_private macros so clang-format understands them 2) Put HDF5 lint back to the right place 3) Fix includes: sorting the includes via clang-format uncovered an include error.
852e1ce
to
0404103
Compare
Merged to I will need to apply this in the right order to the 0.14.5 branch, otherwise back-porting newer fixes like #1197 becomes very tricky :) |
Add a
.clang-format
file to be used for the whole source tree. Close #1024.TODO
OPENPMD_private:
Update:
Using clang-format-12 now Switched to clang-format-10 for now since I couldn't get version 12 running in the CI. Version 12 has some seriously improved handling of lambdas, so we might want to try that again.With pre-commit, we're now using clang-format-12 installed from Conda
--author="Tools <clang@format.com>
for now. Any other suggestion? Axel: ok, but in a separate commit/PR, because we squash PRs. Will cherry-pick on master and let the tool fix it.My personal preferences for formatting:
I don't care too much about tabs vs. spaces, placement of brackets on new lines or at the end of lines.
Guide for rebasing old PRs: ComputationalRadiationPhysics/picongpu#3464
Example for current error message when a wrong formatting is detected: https://github.com/openPMD/openPMD-api/runs/3016610168
Some discussion points:
I have currently specified
NamespaceIndentation=Inner
. This unfortunately also indents something like this:(Adding
CompactNamespaces=true
does not help here since that one is apparently ignored?)EDIT: This situation can be fixed upon
C++17
by simply writingnamespace openPMD::detail {
.Currently, we often use
if( boolean )
. Maybe turn that intoif (boolean)
? These spaces in parens are cumbersome to write and they take up space in nested calls.EDIT: Done this now.