-
Notifications
You must be signed in to change notification settings - Fork 130
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
Create a linter tool to ensure source code formatting #440
Comments
Yes please! I'd prefer adapt our coding style to the closest that clang-format/cpplint can support. Definitely not interested in maintaining yet another custom tool. We've refrained from massive reformatting during porting/refactoring xpcc to modm, since it touches every file and thus breaks the git history, and we needed to extract the copyright authors to relicense modm. |
From http://clang.llvm.org/docs/ClangFormat.html, it seems we have the following code styles to choose between:
I think it is a good idea to only require changed files to adhere to the format. In a few years, we can reformat the rest which haven't seen any changes so far. Should not be too hard, something like Here is a starting point which uses GitHub Actions: https://github.com/cvra/clang-format-action |
To kick this off, I tried the webkit style on my fiber PR. See the result here: henrikssn@328dbb2 I also tried Google style (which I would prefer): henrikssn@2e3c1d1 Any opinions? |
We'll need our own custom clang-format style that is as close as possible to what we have, otherwise we're constantly reformatting files in PRs. |
This is what I got from pasting in a couple of files from modm and poking around the settings until it looked familiar to me again: modm's clang-format style
It's pretty close to what we have, Some of the things are a bit annoying, like braces/alignment/wrapping around short control statements/functions. I guess we're gonna have to live with it. |
Oh and clang-format won't work on the template files. But they have the |
Setting I think we have to live with it being disabled for template classes, I try to minimize using templates anyway since it causes problems with all tooling that assumes C++ syntax (i.e. also my IDE). Is the tab width really 120? That seems quite extreme to me, even Java style only has 100 (to account for the ClassNamesThatAreWayTooLongFactoryImplProvider) |
I think we should test it on parts of the repository to see what config minimizes the changes. Seems easier to me to reverse engineer our actual style that way.
The ColumnLimit is 120 yes, but it's more of a soft recommendation. I often found not brute-forcing the code into a rectangle shape for the sake of it to be better for understanding. |
Hold on, this is not true at all, my editor is set to wrap at 80, with 120 being just a marker line. |
But browsing through the source code, there's a lot of code that's just over 80 and it would hurt readability to force a line break there. I think we should just disable this check completely. |
I'm closing this since we added a |
While most source code roughly follows the guidelines, it has occured to me that there are many cases of inconsistent formatting across the code base. This is annoying, but good news is that it is easily automatable (so that we can focus on more important things, such as my fiber PR #439 :)
Coding convention is here: https://github.com/modm-io/modm/blob/develop/docs/coding_convention.md
This is an ask to either write a linter for the existing style (which we can run in CI), or to switch to a style where there already is a supported linter / formatter tool (my proposal would be Google style, but I am obviously biased).
See https://google.github.io/styleguide/cppguide.html and https://github.com/cpplint/cpplint.
Any opinions here?
The text was updated successfully, but these errors were encountered: