Add .clang-format file which defines coding style as GNU #2851
Conversation
|
Hi @ericcurtin. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Example of code formatted using git clang-format/clang-format and this file: I find this useful, because when different codebases use different style, you can just write it in whatever style you like and auto-format in the end before submission. Before I was spending a lot of time manually editing before submission to ostree. Didn't realize clang-format had GNU these days baked in, nice! |
|
If we wanted we could hook this up to the "Code style" CI build so that all new changes are enforced to use this style, that can be a good compromise instead of re-style the whole codebase. I did that with inotify-tools and never regretted it. But I'd be happy if we could just to have this checked in here for my own personal use and others that like to autoformat with clang-format. |
ericcurtin
force-pushed
the
clang-format
branch
from
4a10789
to
5c49c46
Compare
|
I think the "Debian Testing" failure is just flakey CI |
ericcurtin
force-pushed
the
clang-format
branch
2 times, most recently
from
7d8bb8d
to
d85e5fc
Compare
|
I since realised there's a PR for this already, reworked to be just a partial version of: Just the .clang-format file part so people can start using it. |
ericcurtin
force-pushed
the
clang-format
branch
from
d85e5fc
to
c90ee75
Compare
|
I also addressed this comment by @dbnicholson from #2565:
Although I didn't use a plugin to do it, I try to avoid CI plugins because if you ever have to migrate off GitHub or somebody stops maintaining the plugin, it's a pain. But if you guys would prefer to use a pre-baked plugin, that would work too. |
ericcurtin
force-pushed
the
clang-format
branch
19 times, most recently
from
21383fa
to
8c4a3b9
Compare
ericcurtin
force-pushed
the
clang-format
branch
4 times, most recently
from
2875603
to
e33391c
Compare
This is particularly useful for those that work on multiple projects with different styles so they can easily auto-format their code before submission. Luckily libostree looks like it's pretty standard GNU style so this file is quite small. I also added a CI check so all new code follows the style defined in the .clang-format file.
ericcurtin
force-pushed
the
clang-format
branch
from
e33391c
to
e24c2af
Compare
| @@ -1,7 +1,27 @@ | |||
| #!/usr/bin/env bash | |||
| # Tests that validate structure of the source code; | |||
| # can be run without building it. | |||
| set -euo pipefail | |||
| set -eo pipefail | |||
There was a problem hiding this comment.
I'd prefer we keep this and instead...
| set -euo pipefail | ||
| set -eo pipefail | ||
|
|
||
| if command -v clang-format >/dev/null && [ -n "$GITHUB_ACTOR" ]; then |
There was a problem hiding this comment.
...use [ -n "${GITHUB_ACTOR:-}" ]
| git rebase $THAT_COMMIT | ||
|
|
||
| echo -n "Formatting with git clang-format... " | ||
| if ! git clang-format $THAT_COMMIT; then |
There was a problem hiding this comment.
What makes this a git subcommand?
| @@ -27,7 +27,7 @@ set -euo pipefail | |||
| set -x | |||
|
|
|||
| # First, basic static analysis | |||
| ./ci/codestyle.sh | |||
| ./ci/codestyle.sh clang-format | |||
There was a problem hiding this comment.
I can remove the "clang-format" arg if needs be. I added it because I felt the ci/codestyle.sh shouldn't do git operations like fetch, rebase, etc. and apply reformats by default. But it's the only way I could thing of, of getting git clang-format to just look at the changes in a PR. If we reformat the whole codebase though, you can simply call clang-format to check the whole codebase.
Another thing I've seen happen, just to be mindful of it if we start seeing it, is sometimes you have to do version checks eventually like only check if clang-format version is greater than 12. Because sometimes how it reformats between versions can change slightly, and you get into the situation where so some builds thing the style is ok, and some don't depending on clang-format version.
This is the way I did it in inotify-tools CI, is we only support clang-format greater than 11 because earlier versions reformat a little differently:
I didn't reformat the whole codebase in inotify-tools. Like I am proposing here, all new changes to inotify-tools are mandated to correspond with the .clang-format style. It avoids the "git blame" being confusing problem.
Yeah happy to close this |
This is particularly useful for those that work on multiple projects
with different styles so they can easily auto-format their code before
submission. Luckily libostree looks like it's pretty standard GNU style
so this file is quite small.