Hi everyone!
I wanted to raise a question about code style for the project. I've been
receiving some feedback on PRs about the need to:
- Have curly braces start on a new line
- Remove curly braces if the condition or loop has only one expression
Taking a look at the official Code Style stated in the web, I read that:
"{ and } are placed on a new line except when empty or opening a multi-line
lambda expression. Braces may be elided to a depth of one if the condition or
loop guards a single expression."
Which addresses the first type of comments I mentioned (curly braces starting
in a new line), but leaves open the second type of comments (remove not needed
curly braces).
But, when looking at the checkstyle.xml, I don't see any rule enforcing any of
those two types of comments.
I believe checkstyle.xml should be our contract, so I'm proposing here:
For "curly braces starting in a new line" rule, add something like what we
already have on Sidecar and Analytics projects:
<module name="LeftCurly">
<!-- Checks for placement of the left curly brace ('{'). -->
<property name="option" value="nl"/>
...
</module>
That way, we can fail fast and not worry about those comments on PRs. This of
course may be painful, as we probably will have to fix a bunch of wrongly
placed brackets all over the place.
If there are no concerns here, I'll be more than happy to bite the bullet and
add a patch for this.
For "remove not needed curly braces", I understand that it tends to be the
preference on the code, so we either modify the documentation and add a rule
for that on the checkstyle.xml, or we are fine with that style and there is no
need to remove them on patches.
I wanted to hear the thoughts on the community for this one. My preference is
to always use brackets, but that's just a preference, so it's perfectly fine
not to enforce it and leave the documentation as is.
Thanks everyone!
Bernardo
