You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Suraj Khurana <su...@hotwax.co> on 2020/07/13 15:33:00 UTC
Single line statements - checkstyle
Hello team,
I have created a ticket https://issues.apache.org/jira/browse/OFBIZ-11886
to allow single line statements during checkstyle, I hope we are fine with
this.
For this, we need to add a module to allow single line statements:
==============
<module name="NeedBraces">
<property name="allowSingleLineStatement" value="true"/>
</module>
==============
Let me know your thoughts on this.
--
Best Regards,
Suraj Khurana
Senior Technical Consultant
Re: Single line statements - checkstyle
Posted by Suraj Khurana <su...@hotwax.co>.
Thanks everyone.
This was done a few days ago.
--
Best Regards,
Suraj Khurana
Senior Technical Consultant
On Sat, Jul 18, 2020 at 1:56 PM Jacques Le Roux <
jacques.le.roux@les7arts.com> wrote:
> Le 14/07/2020 à 10:15, Jacques Le Roux a écrit :
> > My conclusion is that could be discussed again and I for one I'm not
> against changing this rule for 2 reasons:
> >
> > 1. We can't use our current formatter to do it right
> > 2. With a single line statement there are less chances to confuse
> and fall in the error-prone form mentioned above
> >
> > Another option would be to find a way to improve our formatter to do the
> right thing (ie add braces when splitting).
>
> We discussed the point 1 with Suraj in Slack. It's actually possible in
> both IntelliJ
>
>
> https://www.jetbrains.com/help/resharper/Braces_for_Single_Line_Statements.html
>
> https://stackoverflow.com/questions/55248370/intellij-checkstyle-plugin-reformat
>
> and Eclipse
>
>
> https://stackoverflow.com/questions/3704308/how-to-make-eclipse-automatically-add-braces-to-an-if-statement
>
> I don't know for IntelliJ, but in Eclipse it's not a formatting option but
> a clean-up option.
> And when you clean-up you can't select a block of code, it's all the file
> or nothing.
> There are then more changes and some are unwanted.
>
> So I agree with Suraj and Pritam, let's do that. There would be anyway so
> much changes to do by hand that it's not rational!
>
> Jacques
>
>
Re: Single line statements - checkstyle
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 14/07/2020 à 10:15, Jacques Le Roux a écrit :
> My conclusion is that could be discussed again and I for one I'm not against changing this rule for 2 reasons:
>
> 1. We can't use our current formatter to do it right
> 2. With a single line statement there are less chances to confuse and fall in the error-prone form mentioned above
>
> Another option would be to find a way to improve our formatter to do the right thing (ie add braces when splitting).
We discussed the point 1 with Suraj in Slack. It's actually possible in both IntelliJ
https://www.jetbrains.com/help/resharper/Braces_for_Single_Line_Statements.html
https://stackoverflow.com/questions/55248370/intellij-checkstyle-plugin-reformat
and Eclipse
https://stackoverflow.com/questions/3704308/how-to-make-eclipse-automatically-add-braces-to-an-if-statement
I don't know for IntelliJ, but in Eclipse it's not a formatting option but a clean-up option.
And when you clean-up you can't select a block of code, it's all the file or nothing.
There are then more changes and some are unwanted.
So I agree with Suraj and Pritam, let's do that. There would be anyway so much changes to do by hand that it's not rational!
Jacques
Re: Single line statements - checkstyle
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Suraj,
We discussed this in the past (I can't find it yet) and we concluded that we should not allow that.
It's in contradiction with "Code Conventions for the Java^TM Programming Language":
https://www.oracle.com/java/technologies/javase/codeconventions-statements.html#449
Hence with our recommended conventions as in
https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions
and
https://gitbox.apache.org/repos/asf?p=ofbiz-tools.git;a=blob_plain;f=wiki-files/OFBizJavaFormatter.xml
BTW, I was looking at LoginServices::checkNewPassword. Its signature was outrageous long (234 chars!), I splitted it by hand.
I wanted to commit it as is but while at it I decided to try to format the whole class with Eclipse.
I just putt the result as OFBIZ-11886-format-LoginServices-checkNewPassword.patch under OFBIZ-11886.
I then noticed that Eclipse rightly splits "if single line statements" but does not add braces, {}. It's not good as it does not follow Sun convention:
Note: if statements always use braces, {}. Avoid the following error-prone form:
if (condition) //AVOID! THIS OMITS THE BRACES {}!
statement;
With all this said.
* We have tons of single line statements in OFBiz.
* It's certainly part of much of errors reported by checkstyle.
* I don't remember clearly but it seems to me that when we discussed it last time it was a moot point. Like with return on the same line.
My conclusion is that could be discussed again and I for one I'm not against changing this rule for 2 reasons:
1. We can't use our current formatter to do it right
2. With a single line statement there are less chances to confuse and fall in the error-prone form mentioned above
Another option would be to find a way to improve our formatter to do the right thing (ie add braces when splitting).
My 2 cts
Jacques
||
Le 14/07/2020 à 07:30, Pritam Kute a écrit :
> Looks good to me.
>
> Kind Regards,
> --
> Pritam Kute
>
>
> On Mon, Jul 13, 2020 at 9:04 PM Suraj Khurana <su...@hotwax.co>
> wrote:
>
>> Hello team,
>>
>> I have created a ticket https://issues.apache.org/jira/browse/OFBIZ-11886
>> to allow single line statements during checkstyle, I hope we are fine with
>> this.
>>
>> For this, we need to add a module to allow single line statements:
>> ==============
>> <module name="NeedBraces">
>> <property name="allowSingleLineStatement" value="true"/>
>> </module>
>> ==============
>>
>> Let me know your thoughts on this.
>>
>> --
>> Best Regards,
>> Suraj Khurana
>> Senior Technical Consultant
>>
Re: Single line statements - checkstyle
Posted by Pritam Kute <pr...@hotwaxsystems.com>.
Looks good to me.
Kind Regards,
--
Pritam Kute
On Mon, Jul 13, 2020 at 9:04 PM Suraj Khurana <su...@hotwax.co>
wrote:
> Hello team,
>
> I have created a ticket https://issues.apache.org/jira/browse/OFBIZ-11886
> to allow single line statements during checkstyle, I hope we are fine with
> this.
>
> For this, we need to add a module to allow single line statements:
> ==============
> <module name="NeedBraces">
> <property name="allowSingleLineStatement" value="true"/>
> </module>
> ==============
>
> Let me know your thoughts on this.
>
> --
> Best Regards,
> Suraj Khurana
> Senior Technical Consultant
>