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 |
| repos: | ||
| - repo: https://github.com/franzpoeschel/openPMD-api-pre-commit-hooks | ||
| rev: aa38b2ad4fae42db05c19de211f43df6a7981bc5 | ||
| - repo: local |
There was a problem hiding this comment.
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.
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.
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-formatfile 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=truedoes not help here since that one is apparently ignored?)EDIT: This situation can be fixed upon
C++17by 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.