Comments (17)
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:
- In projects maintained by one developer, like often happens, having a specific
.clang-format
for that project looks a good compromise - In bigger projects (like
yarp
) a global.clang-format
could be potentially enforced, and the discussion can happenin this gistin this issue. - 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.
Related Issues (20)
- [HumanLogger] Logger silently failing to save file when attach device not found HOT 15
- Remove teleoperation models for icub2.5 and icub3 HOT 2
- Remove implementation of both IWrapper and IMultipleWrapper from devices HOT 2
- Add joint information to HumanLogger HOT 2
- Migrate MacOS CI from homebrew to conda HOT 2
- Human Logger not terminating when directory not created HOT 3
- [HumanLogger] logger not starting because interface is not ready HOT 2
- Replace usage of `analogServer` HOT 17
- Change Wrapper/Remapper naming convention to `nws`/`nwc` HOT 2
- Add status information to `IHumanState`, `IHumanDynamics` and `IHumanWrench`
- Implement HumanWrenchWrapper and HumanWrenchRemapper devices HOT 1
- HumanControlBoard fails to attach to iHumanState HOT 2
- HumanStateVisualizer is stuck waiting data from HumanStateWrench HOT 2
- CI failing on Windows HOT 3
- Release human-dynamics-estimation v3.0.0 HOT 2
- Add video recording in HumanLogger HOT 1
- cannot set `useDirectBaseMeasurement` to true HOT 1
- Compilation failure with ``yarp`` 3.10
- RViz crashes when launched by yarpmanager
- Re-enable macos and Windows CI April 2024 HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from human-dynamics-estimation.