clang-format whole project + new CI workflow for checking style #2152
Conversation
piponazo
requested review from
clanmills,
neheb,
kevinbackhouse,
kmilos,
hassec and
postscript-dev
|
|
||
| #endif // #ifndef XMPSIDECAR_HPP_ | ||
| #endif // #ifndef XMPSIDECAR_HPP_ |
There was a problem hiding this comment.
These ending comments I find interesting. In other codebases that I've seen they just have the define name as a comment. I think google's cpplint also complains about it.
There was a problem hiding this comment.
I am not sure what is your point here. clang-format is just reformatting here the spaces between the #endif and the comment.
Codecov Report
@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
- Coverage 63.25% 62.65% -0.60%
==========================================
Files 97 97
Lines 19154 19722 +568
Branches 9722 9710 -12
==========================================
+ Hits 12116 12357 +241
- Misses 4779 5133 +354
+ Partials 2259 2232 -27
Continue to review full report at Codecov.
|
app/exiv2app.hpp
Outdated
| struct CmdIdAndString { | ||
| CmdId cmdId_; //!< Commands identifier | ||
| std::string cmdString_; //!< Command string | ||
| struct CmdIdAndString |
There was a problem hiding this comment.
Feels like a lot of these can be converted to std::pair + structured bindings.
There was a problem hiding this comment.
Please, let's focus in this PR in the clang-format style. I am sure you will find many opportunities here for refactoring but these should be addressed in other PRs 😉
| { "Sensing method", Exiv2::sensingMethod }, | ||
| { "AF point", Exiv2::afPoint } | ||
| }; | ||
| static const EasyAccess easyAccess[] = {{"Orientation", Exiv2::orientation}, |
There was a problem hiding this comment.
formatting here is nasty. Can be fixed by adding a comma at the end of the last member.
There was a problem hiding this comment.
I agree on this. I invested some time trying to find a way to split the arrays in an homogeneous way (keeping always the same indentation, separating each item in a different line, etc) but it looks difficult to achieve. I was specially playing with the file src/fujimn_int.cpp.
I'll keep investigating how to make the formatting of arrays more "beautiful". Suggestions here are welcome!
There was a problem hiding this comment.
In the last commit (ad7f178) I wanted to show you what's the effect of adding a comma after the last item in the array.
In most of the cases, it makes the formatting better (see fujiOffOn, fujiTone and others) and it split each item in a different line. However, there are some cases in which clang-format decides to "pack" several items in the same line (see fujiSharpness or fujiFlashMode). That's something I would really like to change if it is possible. I would like to see a more consistent behavior for all the cases.
There was a problem hiding this comment.
You can work around it by adding // at the end of each entry.
The issue interestinly enough has to do with the max characters per line. If you remove that it won't pack them.
There was a problem hiding this comment.
I really don't like adding // at the end of the lines. It sets a bad example, as people will then just start using comments to hack around what clang-format does.
There was a problem hiding this comment.
I followed your suggestion and I really like the result. You can see the changes in the new commit 5ef2f8b. I played there with the file canonmn_int.cpp. I applied some manual changes to that file so that the array elements are split in different lines:
- Add extra comma after last item in the arrays
- If a comment is present in the last item (i.e // To silence warnings), then we also need to add a comment after the first item.
For the moment I only did it for one file. If we agree on this format, I will happily apply such changes to the rest of the project.
There was a problem hiding this comment.
@hassec I just had to add the // in few cases. If none of the items have comments at their right side, then it is not needed to add more comments. I just had to do that trick in cases like the following one, where there was a unique comment in the last item of the array: 5ef2f8b#diff-8e58de4a7a00f0a4e00754c14143edab88e415b0e92b8545f977c5b1fe478681R1407.
I agree with you that we should not start adding many of such "tricks" and expect from contributors to know them or apply them.
On the other hand, our codebase is full of such arrays with many items inside, and I would really like to have a consistent formatting for them.
There was a problem hiding this comment.
I'm not sure if I like setting ColumnLimit : 0 as that disables e.g. the reformatting of functions etc.
take this example:
// if limit is 120
int test(uint32_t LongNameForParameter1, double LongNameForParameter2, const float &LongNameForParameter3);
// if limit is 80
int test(uint32_t LongNameForParameter1, double LongNameForParameter2,
const float &LongNameForParameter3);this won't get reformatted to a common form as clang-format will simply use the input provided spacing if you set ColumnLimit to 0
There was a problem hiding this comment.
Right, that was also a concern I had. I think that what we can see right now is Okayish because I firstly applied clang-format to the whole project with the ColumnLimit set to 120 and in the last commits (with ColumnLimit=0) it kept the formatting somehow.
It would probably better to just keep the .clang-format with the original version. The only drawback is that some arrays are not "perfectly" formatted.
Keep your suggestions & thought flowing! Thanks for all the feedback ❤️
|
I have always supported the concept of reformatting the code when we reach 1.0 and when the 0.27-maintenance branch is at end-of-life. When we agreed this in May 2018, we did not expect 0.27-maintenance to be the shipping branch in 2022. @alexvanderberkel has proposed a Team Meeting. We should discuss how to ship 'main' and how to end-life 0.27-maintenance. |
|
@clanmills in commit 575826d I removed some sample applications which were not considered in our CMake code. It looks like you were the author of most of them. Do you think we should keep some of them? (if so, I will adapt the cmake code to compile them) |
|
Thanks for working on this @piponazo, I'd love to see this. Regarding the style to use, is there any downside of just picking one of the default styles? Would avoid even opening the pandora's box of discussing individual settings. |
That's a good way of thinking. I'll play with the different styles and check if some of them is similar to what we currently have. Any preferences? |
|
no strong preference, I'd be happy with any of them 👍 |
piponazo
force-pushed
the
mainClangFormat
branch
from
43388a2
to
ef56ca1
Compare
I have been playing with the different styles, but no one is good enough for me without some customisation. At the end I ended up using the customised options we already had in the |
|
Suggestion for reviewing the changes: "Use the hide whitespace" option |
There was a problem hiding this comment.
couple default values that I'd remove
The comments about which style to pick is just to voice it. This is exactly the tricky bit of subjectivity that I think isn't worth it and why I would have just picked plain Google plus the ones that have coverage implications.
Leaving it up to you what to do 😉 I'm just happy that we will have a style 🎉
|
@hassec I applied all your suggestions except for the removal of |
|
I agree with @hassec about keeping it close to the given style. The current extras make sense. |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: RafikFarhad/clang-format-github-action@v2.0.0 |
There was a problem hiding this comment.
@piponazo have you considered: https://github.com/DoozyX/clang-format-lint-action
Just asking because I noticed that that repo has:
- more stars, for whatever that is worth 😂
- option to explicitly specify the clang-format version which is quite useful to ensure stable output and we could directly use version 13
- ability to directly apply the formatting and commit it
It would also replace the need for the bash script in this PR.
The biggest benefit might be that we could skip the checking of PRs and simply run one action that runs clang-format on master every time something is merged. This might be a more frictionless workflow compared to a PR CI action which fails due to formatting error and then the user needs to manually fix that formatting violation.
Especially as the user can only fix it if he/she has the right version of clang-format installed
There was a problem hiding this comment.
The truth is that I just used the first Github Action that I encountered. I'll try this one out 👍
There was a problem hiding this comment.
I have been playing a bit around with clang-format-lint-action and it works quite well! It runs faster than the other one and as you said we can think about integrating an automatic action to automatically apply clang-format before merging a PR.
However, I would wait a bit for taking the step of introducing automatic commits related to the clang-format ... it scares me a bit what could happen without having some weeks/months of experience with the new clang-format settings we are defining here.
What I can definitely do is to write some documentation about the implications of enforcing clang-format now in the project in the file CONTRIBUTING.md.
There was a problem hiding this comment.
@piponazo You are probably right that it's better to gain some experience with the action before we let it automatically commit to master.
How would you propose the workflow should look?
Let's say a user opens a PR and the pipeline fails, what would you suggest to them they do?
What if they don't have clang-format?
Or like in my case my system comes with
❯ clang-format --version
clang-format version 13.0.1That means for me there will be cases where I won't be able to fix the pipeline without installing a new clang-format version or trying to figure out the correct diff to apply manually from the failing ci log.
All of that sounds like a bit too much friction to me.
What if additionally to the check that you implemented, we add one more action which is triggered by a specific comment.
E.g. if a comment includes ExivBot plz format the action will be triggered and applies the correct formatting to the PR.
Edit, also just found: https://github.com/Khan/pull-request-comment-trigger which could be useful, but it's not maintained for > 2 years.... And it seems like we can do it relatively easy ourselves: https://github.51.almunity/t/trigger-a-github-workflow-if-it-matches-a-particular-comment-in-the-pull-request/116402
There was a problem hiding this comment.
I was thinking about this yesterday when I went to bed (I think I have a problem 🛌 ).
If a contributor creates a PR and the CI pipeline fails (this might happen not only to new contributors but also to experienced ones) we will have a a CI job indicating exactly what is the problem. At that point we can point them to the CONTRIBUTING.md file which has some notes about applying clang-format before summiting code.
Regarding the clang-format versions. I tried yesterday and clang-format-12 & clang-format-13 are producing the same results with the .clang-format file we have here. Of course, if they do not have clang-format installed, they will need to go to the LLVM download page and download the binaries for their platform: https://releases.llvm.org/download.html. I agree that this might create some friction for people who never used clang-format before. But hey if they managed to install git, compilers, cmake, conan, etc ... they should manage to install one tool more 😋 .
The main problems I see with the automatic approach are the following:
- A contributor creates a PR and we will not have a clang-format check at that point (the automatic formatting will happen after the PR is approved and merged). Since we have no formatting checks when the PR is created, we will see PRs that might contain some crazy formatting , reviewers might start to discuss again about formatting style, or it is simply more difficult to review the code because we see different formatting styles in each PR. For me that is one of the most important points about brining a unified and enforced clang-format: to ease the coding reviews.
- Let's say that we can "live" with the previous point. After the PR is approved and we click the merge button, the automatic commit happens. At this point at least 2 things might go wrong:
- The automatic commit produce some changes that break the compilation. This barely happens but the probability is not 0%. It happened twice to me while working on this branch: Once due to the order of headers, and other time with some problem with preprocessor code and comments.
- The 2 problem I see is that we might not like the automatic formatting applied to the code and we have no chance to adapt the code in the PR (the PR will be already merged!). By forcing to have the clang-format job passing before merging, we have the chance to still make some suggestions about formatting during the coding review. For example: adding a comma after the last element of an array to have a nicer style after clang-format is applied.
- We can end up having too many commits just applying clang-format and when you try to use
git blameyou end up seeing that many areas of the code were finally touched by a "ROBOT". I think that this is bad and should be avoided as much as possible. We should also recommend contributors to apply clang-format before committing. Or if they forget, it is always possible to do an interactive rebase and squash some commits together.
There was a problem hiding this comment.
Behavior between versions can definitely change, we've experienced this at work.
Thus, to be on the safe side one has to have the correct version, which is something I don't like. I personally don't want to have to download a specific version of clang-format for exiv2 and then make my editor somehow pick that version over my default one.
I think the contributing guidelines can still say to apply clang-format if it is available on a user's system. That get's a good starting point for a review
Otherwise, I agree with some of your points about the automatic commit, but my last comment was about a manually triggered action on a PR.
This could be applied before a review if the original formatting is terrible, or at the end before merging.
But it would still enable a review of the formatting before merging and it would trigger the CI so we know if it compiles.
And the problem of having these commits in the history, yes that is a bit annoying but in most cases we can probably just squash & merge and thus won't have those commits.
A PR that is big enough to have many commits that shouldn't be squashed probably comes from a user who format and/or rebase themselves.
There was a problem hiding this comment.
Right, I think the approach about having a comment to trigger the clang-format in the PR might be a good solution to achieve the best of both sides.
I'll switch this PR back to draft state and play a bit with that 😉
piponazo
added
CI
piponazo
force-pushed
the
mainClangFormat
branch
3 times, most recently
from
61202f4
to
33fb2ba
Compare
piponazo
force-pushed
the
mainClangFormat
branch
2 times, most recently
from
69c88e6
to
d0fd396
Compare
piponazo
force-pushed
the
mainClangFormat
branch
from
d0fd396
to
30bf563
Compare
Over the last years I discussed or mention to different people the possibility/dream to apply the clang-format style over the whole project and enforce the strict usage of the style by adding some CI checks or by any other means. With this Draft I would like to propose you to finally do it.
Thanks to this Github action, the effort I had to do is minimal. We just need to add a new workflow which uses that action, and we need to indicate which folders and files to inspect. Then the action uses our
.clang-formatfile to make sure all the files have the proper format. You can see the result here:https://github.com/Exiv2/exiv2/actions/runs/1989475924
Over the last years, the
.clang-formatfile evolved as different people proposed different customisations. In this branch you have the opportunity to propose & discuss further changes to the format style. Once we reach an agreement we should avoid to touch that style again as much as possible.Any other suggestions regarding clang-format and its enforcement for future contributions is welcome.
Possible future work