Automatically format code (clang-format) about human-dynamics-estimation HOT 17 CLOSED

I'm not aware of the background behind this praiseworthy initiative, so I might be missing important details, but let me raise one point 'cause I can see there may be some concerns, at least with me 😉

I have nothing against defining official code styles for e.g. yarp or specific project repositories, but I'm a bit skeptical - to say the less - to sort of blindly extend these guidelines to all repositories in our ecosystem.

I'm a bit "conservative" with my own code style ─ to soften my religious truth of "nothing of this world can convince me to use { inline with if or to wrap one single operation" ─, but at the same time I also hate seeing modules written with different styles, of course, so that I obviously adapt my contributions to the maintainer's choice when opening a PR on a repo that I don't maintain.

The role of the maintainer is the key in my view. Therefore, for most of our repositories where a widely agreed style is not selected (thus excluding yarp and a few others), the maintainer(s) should check the style of the contributed code and push developers to react accordingly, even by means of automatic checkers.

I don't think it's feasible to force people adhering to one single style everywhere, while it would be beneficial ending up with multiple coherent code styles, depending on the diverse maintainers, who are committed to enforcing them.

from human-dynamics-estimation.

-1000 for tabs

from human-dynamics-estimation.

Why instead of starting with a huge file full of settings, don't we start with a "blank" one and we discuss and add one by one each setting? reviewing such a file is impossible

from human-dynamics-estimation.

I created this gist this issue. Having a demo repository only for this is probably too much, let me know if you have better ideas. Spread this out.

@traversaro @francesco-romano @drdanz @barbalberto @randaz81 @pattacini @Nicogene @aerydna

from human-dynamics-estimation.

As I wrote in the gist, how to enforce a style is an even more open question. In my opinion using these tools has become a good practice of coding that we cannot neglect anymore. Style consistency in the code is synonym of seriousness from an external point of view, and during my workflow I find useful and time saving having the possibility to automatically format the style every time I save the file. (e.g. when I forget a ; in the previous line, I can put it in the beginning of the next and it is automatically moved above when I save).

To address your concern, in my opinion we all should have a global style guideline and enforce it for repos with more traffic. This global clang-format can be installed globally (maybe saved in a robotology repository so people can pull changes) and used as default. Then, for project-specific style, a specific clang-format saved in the root of the folder will have the priority on top of the global one. Many IDEs already support this behavior. This local file can match more the style of the maintainer, and we know that many small repos of our ecosystem have just one maintainer (if any). I believe that this discussion could be useful to attract interest and make people aware of this tool, and then for applying such policies we have a big room of play. This approach could hopefully minimize the changes that maintainers apply on their repositories, since their starting point would be the global configuration.

Trying to extend further my point, what I am looking for is at least a local clang-format. Honestly I cannot waste time trying to figure out the maintainer style, overall when I work on multiple repos, and receiving requests of style changes before merging PRs. It is wasted time from both sides, and I hope many will agree on this. Developers should focus on developing. Of course that is my 2cents.

from human-dynamics-estimation.

@diegoferigo 👍 for the global (but not restrictive) style + local styles.

from human-dynamics-estimation.

@diegoferigo just to give you another reason to always use spaces instead of tabs, please be aware that according to statistics, programming with spaces instead of tabs makes you richer!!!

from human-dynamics-estimation.

Also this http://clang-format.me/

from human-dynamics-estimation.

I'd like to bump again this discussion. I'm really tired to see a messy style in some code, with many style differences present even in the same file. For the time being, we can start a chat on a clang-format that everyone like(ish), keeping in mind that we will never find a solution that makes all happy.

In my opinion we can reach a draft here on repositories that we directly handle, and then extend this discussion to all other developers. This would allow to start at least with a small group on the same line.

This is my clang-format:

---
BasedOnStyle: WebKit
AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: 'true'
AlignConsecutiveDeclarations: 'false'
AlignOperands: 'true'
AlignTrailingComments: 'true'
AllowAllParametersOfDeclarationOnNextLine: 'true'
AllowShortBlocksOnASingleLine: 'true'
AllowShortCaseLabelsOnASingleLine: 'false'
AllowShortFunctionsOnASingleLine: Inline
AllowShortIfStatementsOnASingleLine: 'false'
AllowShortLoopsOnASingleLine: 'false'
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: 'false'
AlwaysBreakTemplateDeclarations: 'true'
BinPackArguments: 'false'
BinPackParameters: 'false'
BraceWrapping:
  AfterClass: false
  AfterControlStatement: false
  AfterEnum: true
  AfterFunction: true
  AfterNamespace: false
  AfterObjCDeclaration: false
  AfterStruct: false
  AfterUnion: false
  BeforeCatch: false
  BeforeElse: true
  IndentBraces: false
BreakBeforeBraces: Custom
BreakConstructorInitializersBeforeComma: 'true'
ColumnLimit: '100'
ConstructorInitializerAllOnOneLineOrOnePerLine: 'true'
ContinuationIndentWidth: '10'
Cpp11BracedListStyle: 'true'
DerivePointerAlignment: 'false'
DisableFormat: 'false'
ExperimentalAutoDetectBinPacking: 'false'
FixNamespaceComments: 'true'
IndentCaseLabels: 'true'
IndentWidth: '4'
KeepEmptyLinesAtTheStartOfBlocks: 'false'
Language: Cpp
NamespaceIndentation: None
PointerAlignment: Left
ReflowComments: 'true'
SortIncludes: 'true'
SpaceBeforeAssignmentOperators: 'true'
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: 'false'
SpacesInAngles: 'false'
SpacesInParentheses: 'false'
SpacesInSquareBrackets: 'false'
Standard: Cpp11
TabWidth: '4'
UseTab: Never
...

The diff wrt the file from @francesco-romano is the following:

--- clang-format-fraromano        2017-06-15 10:12:44.882058574 +0200
+++ clang-format-diego        2017-05-04 10:59:37.513447275 +0200
@@ -1,96 +1,59 @@
----
-Language:        Cpp
-# BasedOnStyle:  WebKit
-AccessModifierOffset: -4
+---
+BasedOnStyle: WebKit
 AlignAfterOpenBracket: Align
-AlignConsecutiveAssignments: false
-AlignConsecutiveDeclarations: false
-AlignEscapedNewlinesLeft: false
-AlignOperands:   false
-AlignTrailingComments: false
-AllowAllParametersOfDeclarationOnNextLine: true
-AllowShortBlocksOnASingleLine: true
-AllowShortCaseLabelsOnASingleLine: false
-AllowShortFunctionsOnASingleLine: All
-AllowShortIfStatementsOnASingleLine: false
-AllowShortLoopsOnASingleLine: false
+AlignConsecutiveAssignments: 'true'
+AlignConsecutiveDeclarations: 'false'
+AlignOperands: 'true'
+AlignTrailingComments: 'true'
+AllowAllParametersOfDeclarationOnNextLine: 'true'
+AllowShortBlocksOnASingleLine: 'true'
+AllowShortCaseLabelsOnASingleLine: 'false'
+AllowShortFunctionsOnASingleLine: Inline
+AllowShortIfStatementsOnASingleLine: 'false'
+AllowShortLoopsOnASingleLine: 'false'
 AlwaysBreakAfterDefinitionReturnType: None
 AlwaysBreakAfterReturnType: None
-AlwaysBreakBeforeMultilineStrings: false
-AlwaysBreakTemplateDeclarations: false
-BinPackArguments: true
-BinPackParameters: true
-BraceWrapping:   
-  AfterClass:      true
+AlwaysBreakBeforeMultilineStrings: 'false'
+AlwaysBreakTemplateDeclarations: 'true'
+BinPackArguments: 'false'
+BinPackParameters: 'false'
+BraceWrapping:
+  AfterClass: false
   AfterControlStatement: false
-  AfterEnum:       true
-  AfterFunction:   true
-  AfterNamespace:  true
+  AfterEnum: true
+  AfterFunction: true
+  AfterNamespace: false
   AfterObjCDeclaration: false
-  AfterStruct:     true
-  AfterUnion:      true
-  BeforeCatch:     false
-  BeforeElse:      false
-  IndentBraces:    false
-BreakBeforeBinaryOperators: All
+  AfterStruct: false
+  AfterUnion: false
+  BeforeCatch: false
+  BeforeElse: true
+  IndentBraces: false
 BreakBeforeBraces: Custom
-BreakBeforeTernaryOperators: true
-BreakConstructorInitializersBeforeComma: true
-BreakAfterJavaFieldAnnotations: false
-BreakStringLiterals: true
-ColumnLimit:     0
-CommentPragmas:  '^ IWYU pragma:'
-ConstructorInitializerAllOnOneLineOrOnePerLine: true
-ConstructorInitializerIndentWidth: 0
-ContinuationIndentWidth: 4
-Cpp11BracedListStyle: false
-DerivePointerAlignment: false
-DisableFormat:   false
-ExperimentalAutoDetectBinPacking: false
-ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH ]
-IncludeCategories: 
-  - Regex:           '^"(llvm|llvm-c|clang|clang-c)/'
-    Priority:        2
-  - Regex:           '^(<|"(gtest|isl|json)/)'
-    Priority:        3
-  - Regex:           '.*'
-    Priority:        1
-IncludeIsMainRegex: '$'
-IndentCaseLabels: true
-IndentWidth:     4
-IndentWrappedFunctionNames: false
-JavaScriptQuotes: Leave
-JavaScriptWrapImports: true
-KeepEmptyLinesAtTheStartOfBlocks: true
-MacroBlockBegin: ''
-MacroBlockEnd:   ''
-MaxEmptyLinesToKeep: 1
-NamespaceIndentation: Inner
-ObjCBlockIndentWidth: 4
-ObjCSpaceAfterProperty: true
-ObjCSpaceBeforeProtocolList: true
-PenaltyBreakBeforeFirstCallParameter: 19
-PenaltyBreakComment: 300
-PenaltyBreakFirstLessLess: 120
-PenaltyBreakString: 1000
-PenaltyExcessCharacter: 1000000
-PenaltyReturnTypeOnItsOwnLine: 60
+BreakConstructorInitializersBeforeComma: 'true'
+ColumnLimit: '100'
+ConstructorInitializerAllOnOneLineOrOnePerLine: 'true'
+ContinuationIndentWidth: '10'
+Cpp11BracedListStyle: 'true'
+DerivePointerAlignment: 'false'
+DisableFormat: 'false'
+ExperimentalAutoDetectBinPacking: 'false'
+FixNamespaceComments: 'true'
+IndentCaseLabels: 'true'
+IndentWidth: '4'
+KeepEmptyLinesAtTheStartOfBlocks: 'false'
+Language: Cpp
+NamespaceIndentation: None
 PointerAlignment: Left
-ReflowComments:  true
-SortIncludes:    true
-SpaceAfterCStyleCast: false
-SpaceAfterTemplateKeyword: true
-SpaceBeforeAssignmentOperators: true
+ReflowComments: 'true'
+SortIncludes: 'true'
+SpaceBeforeAssignmentOperators: 'true'
 SpaceBeforeParens: ControlStatements
-SpaceInEmptyParentheses: false
-SpacesBeforeTrailingComments: 1
-SpacesInAngles:  false
-SpacesInContainerLiterals: true
-SpacesInCStyleCastParentheses: false
-SpacesInParentheses: false
-SpacesInSquareBrackets: false
-Standard:        Cpp03
-TabWidth:        4
-UseTab:          Never
+SpaceInEmptyParentheses: 'false'
+SpacesInAngles: 'false'
+SpacesInParentheses: 'false'
+SpacesInSquareBrackets: 'false'
+Standard: Cpp11
+TabWidth: '4'
+UseTab: Never
 ...

Despite my clang-format forces 4 spaces indentation, I'd prefer using tabs for indentations and spaces for alignment. Without an automatic way like this, using this hybrid is a pain and in such a big codebase it would be difficult to maintain, however clang-format makes things easy. Tabs and spaces have their own reason to exists, each with pros and cons, and using them wisely exploiting the best of both is possible. Despite this, I'm gonna start with compromises from my side, I might agree on 4 spaces if this line is strong enough.

@traversaro @francesco-romano @DanielePucci @claudia-lat @gabrielenava @S-Dafarra @nunoguedelha @lucaTagliapietra @Yeshasvitvs

from human-dynamics-estimation.

Agnostic to everything, as long as there is consensus.

The only thing I am strongly against is mixing tabs and spaces in any way.
In a countless use cases (for example when visualizing source files in GitHub web interface, but also when you open a file in a random machine with a random editor with a random operating system) you can't (easily) specify the width of the tabs (on GitHub is always 8 spaces) and this screws up the visualization of any file in which tabs and spaces are mixed assuming that a tabs is 2 or 4 spaces.

from human-dynamics-estimation.

I propose this: let's take a portion of demo code that contains all the cases (even the complicated ones) in the top post (@francesco-romano can I edit your post?), and then slowly vote features in the comments. I'll start with WebKit as base, let me know if and why you don't agree. I'll keep the first post updated.

@drdanz I changed my mind, probably we can start from the very beginning in a more official place, what do you suggest?

from human-dynamics-estimation.

@alecive Let's close the never ending tab vs spaces in this way with a smile 😄 We can't change our minds about, it's always a compromise, I'm already using spaces here xD

So, TL;DR so far:

1. In projects maintained by one developer, like often happens, having a specific .clang-format for that project looks a good compromise
2. In bigger projects (like yarp) a global .clang-format could be potentially enforced, and the discussion can happen in this gist in this issue.
3. For reducing style mismatches, the project-specific .clang-format should use as starting point the global one used in the bigger projects

Any comment?

from human-dynamics-estimation.

Just found out this... https://clangformat.com/

from human-dynamics-estimation.

In the past I used this tool and it is already linked in the gist, but that is another valid alternative. I'd suggest to discuss further steps f2f at the next IFI day.

from human-dynamics-estimation.

cc @diegoferigo https://gist.github.com/diegoferigo/716259a64adb1035e383db3864805ac5#gistcomment-2125058

from human-dynamics-estimation.

@traversaro I didn't receive any notification about your comment, sorry

from human-dynamics-estimation.

Let's switch to robotology-playground/clang-format#1

from human-dynamics-estimation.