You are viewing a plain text version of this content. The canonical link for it is here.
Posted to kerby@directory.apache.org by Stefan Seelmann <ma...@stefan-seelmann.de> on 2015/07/04 16:40:38 UTC
Kerby coding style
Hi all,
I created a basic checkstyle config [1] and fixed some (to me) obvious
issues. Some more checks make sense which require more changes in the
code but I'd like to ask all developers for their opinions. Once we
reach consensus I'd release the checkstyle file and add the check to the
build, in the beginning as deactivated profile or non-failing, once all
issues are fixed activated by default.
So here as quick poll, please express your opinion, my preferred option
is always (1) ;).
LocalVariableName:
Sometimes variable names like 'W1' (Asn1String) or 'Kc' (KcCheckSum) are
used. However variable names should start with lowercase character. I
guess this was done to follow names in specifications.
Poll:
(1) change variable names
(2) disable check
ParameterName:
Same as above (LocalVariableName)
Poll:
(1) change parameter names
(2) disable check
MethodName:
Some test methods contain underscores (e.g.
DecryptionTest.testDecryptDES_CBC_MD4_1)
Poll:
(1) change method names
(2) disable check
AvoidStarImport:
There are many usages of '*' imports, like 'import org.ietf.jgss.*;'
Poll:
(1) force explicit imports or each class
(2) allow star imports and disalbe check
AvoidInlineConditionals
There are 45 inline conditionals (e.g. 'return s != null ?
s.getVersion() : -1;')
Poll:
(1) allow inline condidtionals and disable check
(2) refactor inline conditionals
LineLength:
Poll:
(1) allow line length of 120
(2) force line length of 80
Whitespace:
There are about 1100 whitespace violations
Poll:
(1) follow Sun/Oracle Java convention (requires reformatting with
formatter, see below)
(2) something else
Curly braces:
There are 89 violations
Poll:
(1) follow Sun/Oracle Java convention (requires reformatting with
formatter, see below)
(2) something else
Formatter:
For Eclipse I'd also create a formatter with the following settings:
* Based on Java Conventions with following changes
* 4 spaces, no tabs
* Line length 120 instead of 80
* No comment formatting (Javadoc, block, line)
Poll:
(1) that is fine
(2) something else
Kind Regards,
Stefan
[1]
https://svn.apache.org/repos/asf/directory/buildtools/checkstyle-configuration/trunk/src/main/resources/kerby-checks.xml
RE: Kerby coding style
Posted by "Zheng, Kai" <ka...@intel.com>.
Thanks Stefan,
This is pretty good. Yes let's fix them module by module.
Regards,
Kai
-----Original Message-----
From: Stefan Seelmann [mailto:mail@stefan-seelmann.de]
Sent: Sunday, July 05, 2015 6:20 PM
To: kerby@directory.apache.org
Subject: Re: Kerby coding style
Hi again,
I added checkstyle rules and Eclipse formatting rules to
docs/kerby-checkstyle.xml
docs/kerby-formatting.xml
MethodName and AvoidInlineConditionals are disabled for now. LineLength is configured with 120 to avoid even longer lines, the limit can be changed later.
The checkstyle check is currently not executed in normal Maven build.
To execute the checks run
mvn checkstyle:check
Violations are printed out to command line with
[ERROR]
prefix.
I'd suggest to fix issues module by module.
Later if all issues are fixed we should activate the check by default and let the build fail.
Kind Regards,
Stefan
Re: Kerby coding style
Posted by Stefan Seelmann <ma...@stefan-seelmann.de>.
Hi again,
I added checkstyle rules and Eclipse formatting rules to
docs/kerby-checkstyle.xml
docs/kerby-formatting.xml
MethodName and AvoidInlineConditionals are disabled for now. LineLength
is configured with 120 to avoid even longer lines, the limit can be
changed later.
The checkstyle check is currently not executed in normal Maven build.
To execute the checks run
mvn checkstyle:check
Violations are printed out to command line with
[ERROR]
prefix.
I'd suggest to fix issues module by module.
Later if all issues are fixed we should activate the check by default
and let the build fail.
Kind Regards,
Stefan
RE: Kerby coding style
Posted by "Zheng, Kai" <ka...@intel.com>.
There is a pretty good discussion in Hadoop community for our reference, and we could see that the answer might not be simple.
http://mail-archives.apache.org/mod_mbox/hadoop-common-dev/201505.mbox/%3CFB89B1A5-329F-4D6F-90BD-F9D406B8E650@pandora.com%3E
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Sunday, July 05, 2015 3:42 PM
To: kerby@directory.apache.org
Subject: Re: Kerby coding style
Le 05/07/15 01:36, Zheng, Kai a écrit :
> Hi Stefan,
>
> Thanks for the hard work and sorting this great list out!!
> I'm OK with your preferred options, because most time they're also mine. I'm surprised we have so many exceptions! Just one thing, LineLength. I'm OK to use 120 for the check, but I would suggest we should try to avoid so long lines. Yes, we should never exceed the 120 limit, and if you do, it will fail the building; meanwhile, 80 is encouraged to make it consistent with most of existing codes.
80 chars line length limit is there for historical reasons : VT 100 terminals where 25x 80 chars screen. Nowadays, even on a 15" laptop, I can see around 150 chars on an edited file (in eclipse), even if the edited file is not full screen (ie, I have the package explorer on the left and the Outilne on the right). 120 is really a good limit.
Re: Kerby coding style
Posted by Stefan Seelmann <ma...@stefan-seelmann.de>.
On 07/05/2015 09:41 AM, Emmanuel Lécharny wrote:
> Le 05/07/15 01:36, Zheng, Kai a écrit :
>> Hi Stefan,
>>
>> Thanks for the hard work and sorting this great list out!!
>> I'm OK with your preferred options, because most time they're also mine. I'm surprised we have so many exceptions! Just one thing, LineLength. I'm OK to use 120 for the check, but I would suggest we should try to avoid so long lines. Yes, we should never exceed the 120 limit, and if you do, it will fail the building; meanwhile, 80 is encouraged to make it consistent with most of existing codes.
>
> 80 chars line length limit is there for historical reasons : VT 100
> terminals where 25x 80 chars screen. Nowadays, even on a 15" laptop, I
> can see around 150 chars on an edited file (in eclipse), even if the
> edited file is not full screen (ie, I have the package explorer on the
> left and the Outilne on the right). 120 is really a good limit.
>
FTR: there are 1245 lines longer than 80 characters, when ignoring
Javadoc there are still 1043 lines longer thant 80 characters.
Re: Kerby coding style
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 05/07/15 01:36, Zheng, Kai a écrit :
> Hi Stefan,
>
> Thanks for the hard work and sorting this great list out!!
> I'm OK with your preferred options, because most time they're also mine. I'm surprised we have so many exceptions! Just one thing, LineLength. I'm OK to use 120 for the check, but I would suggest we should try to avoid so long lines. Yes, we should never exceed the 120 limit, and if you do, it will fail the building; meanwhile, 80 is encouraged to make it consistent with most of existing codes.
80 chars line length limit is there for historical reasons : VT 100
terminals where 25x 80 chars screen. Nowadays, even on a 15" laptop, I
can see around 150 chars on an edited file (in eclipse), even if the
edited file is not full screen (ie, I have the package explorer on the
left and the Outilne on the right). 120 is really a good limit.
RE: Kerby coding style
Posted by "Zheng, Kai" <ka...@intel.com>.
Hi Stefan,
Thanks for the hard work and sorting this great list out!!
I'm OK with your preferred options, because most time they're also mine. I'm surprised we have so many exceptions! Just one thing, LineLength. I'm OK to use 120 for the check, but I would suggest we should try to avoid so long lines. Yes, we should never exceed the 120 limit, and if you do, it will fail the building; meanwhile, 80 is encouraged to make it consistent with most of existing codes. I don't wish to argue here why we choose 120 over 80, or otherwise, because it's hard. Many discussion about this occurred elsewhere, like Hadoop community; we would also, but I don't think we have the time. I would rather avoid lengthy discussions for such things and remain our limited energy for the codes. Would this work? Thanks.
Do we need a branch enforcing the check right now so all the guys can help? So many exceptions ... Thanks again for the taking!
Regards,
Kai
-----Original Message-----
From: Stefan Seelmann [mailto:mail@stefan-seelmann.de]
Sent: Saturday, July 04, 2015 10:41 PM
To: kerby@directory.apache.org
Subject: Kerby coding style
Hi all,
I created a basic checkstyle config [1] and fixed some (to me) obvious issues. Some more checks make sense which require more changes in the code but I'd like to ask all developers for their opinions. Once we reach consensus I'd release the checkstyle file and add the check to the build, in the beginning as deactivated profile or non-failing, once all issues are fixed activated by default.
So here as quick poll, please express your opinion, my preferred option is always (1) ;).
LocalVariableName:
Sometimes variable names like 'W1' (Asn1String) or 'Kc' (KcCheckSum) are used. However variable names should start with lowercase character. I guess this was done to follow names in specifications.
Poll:
(1) change variable names
(2) disable check
ParameterName:
Same as above (LocalVariableName)
Poll:
(1) change parameter names
(2) disable check
MethodName:
Some test methods contain underscores (e.g.
DecryptionTest.testDecryptDES_CBC_MD4_1)
Poll:
(1) change method names
(2) disable check
AvoidStarImport:
There are many usages of '*' imports, like 'import org.ietf.jgss.*;'
Poll:
(1) force explicit imports or each class
(2) allow star imports and disalbe check
AvoidInlineConditionals
There are 45 inline conditionals (e.g. 'return s != null ?
s.getVersion() : -1;')
Poll:
(1) allow inline condidtionals and disable check
(2) refactor inline conditionals
LineLength:
Poll:
(1) allow line length of 120
(2) force line length of 80
Whitespace:
There are about 1100 whitespace violations
Poll:
(1) follow Sun/Oracle Java convention (requires reformatting with formatter, see below)
(2) something else
Curly braces:
There are 89 violations
Poll:
(1) follow Sun/Oracle Java convention (requires reformatting with formatter, see below)
(2) something else
Formatter:
For Eclipse I'd also create a formatter with the following settings:
* Based on Java Conventions with following changes
* 4 spaces, no tabs
* Line length 120 instead of 80
* No comment formatting (Javadoc, block, line)
Poll:
(1) that is fine
(2) something else
Kind Regards,
Stefan
[1]
https://svn.apache.org/repos/asf/directory/buildtools/checkstyle-configuration/trunk/src/main/resources/kerby-checks.xml
Re: Kerby coding style
Posted by Stefan Seelmann <ma...@stefan-seelmann.de>.
On 07/06/2015 10:49 AM, Zheng, Kai wrote:
>>> I think it's preferable to diasble the check here. The CIPHEr names use '_'; it would be less readable to remove them
>>> testDecryptDES_CBC_MD4_1 is way better than testDecryptDesCbcMD41
>
> Good catch here! Just wonder if it's doable to only disable the check here like we can in PMD.
Yes, via suppressions file, I'll add one.
Kind Regards,
Stefan
RE: Kerby coding style
Posted by "Zheng, Kai" <ka...@intel.com>.
>> I think it's preferable to diasble the check here. The CIPHEr names use '_'; it would be less readable to remove them
>>testDecryptDES_CBC_MD4_1 is way better than testDecryptDesCbcMD41
Good catch here! Just wonder if it's doable to only disable the check here like we can in PMD.
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Sunday, July 05, 2015 3:38 PM
To: kerby@directory.apache.org
Subject: Re: Kerby coding style
Le 04/07/15 16:40, Stefan Seelmann a écrit :
> Hi all,
>
> I created a basic checkstyle config [1] and fixed some (to me) obvious
> issues. Some more checks make sense which require more changes in the
> code but I'd like to ask all developers for their opinions. Once we
> reach consensus I'd release the checkstyle file and add the check to
> the build, in the beginning as deactivated profile or non-failing,
> once all issues are fixed activated by default.
>
>
> So here as quick poll, please express your opinion, my preferred
> option is always (1) ;).
>
>
> LocalVariableName:
> Sometimes variable names like 'W1' (Asn1String) or 'Kc' (KcCheckSum)
> are used. However variable names should start with lowercase
> character. I guess this was done to follow names in specifications.
> Poll:
> (1) change variable names
> (2) disable check
Obviously (1)
>
>
> ParameterName:
> Same as above (LocalVariableName)
> Poll:
> (1) change parameter names
> (2) disable check
(1) too
>
>
> MethodName:
> Some test methods contain underscores (e.g.
> DecryptionTest.testDecryptDES_CBC_MD4_1)
> Poll:
> (1) change method names
> (2) disable check
I think it's preferable to diasble the check here. The CIPHEr names use '_'; it would be less readable to remove them
testDecryptDES_CBC_MD4_1 is way better than testDecryptDesCbcMD41
>
>
> AvoidStarImport:
> There are many usages of '*' imports, like 'import org.ietf.jgss.*;'
> Poll:
> (1) force explicit imports or each class
> (2) allow star imports and disalbe check
(1)
>
>
> AvoidInlineConditionals
> There are 45 inline conditionals (e.g. 'return s != null ?
> s.getVersion() : -1;')
> Poll:
> (1) allow inline condidtionals and disable check
> (2) refactor inline conditionals
(2) : more readable, remove potential error when those conditional expressions are changed.
>
>
> LineLength:
> Poll:
> (1) allow line length of 120
> (2) force line length of 80
120
>
>
> Whitespace:
> There are about 1100 whitespace violations
> Poll:
> (1) follow Sun/Oracle Java convention (requires reformatting with
> formatter, see below)
> (2) something else
(1)
>
>
> Curly braces:
> There are 89 violations
> Poll:
> (1) follow Sun/Oracle Java convention (requires reformatting with
> formatter, see below)
> (2) something else
(1)
>
>
> Formatter:
> For Eclipse I'd also create a formatter with the following settings:
> * Based on Java Conventions with following changes
> * 4 spaces, no tabs
> * Line length 120 instead of 80
> * No comment formatting (Javadoc, block, line)
> Poll:
> (1) that is fine
> (2) something else
(1)
Thanks Stefan!
Re: Kerby coding style
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 04/07/15 16:40, Stefan Seelmann a écrit :
> Hi all,
>
> I created a basic checkstyle config [1] and fixed some (to me) obvious
> issues. Some more checks make sense which require more changes in the
> code but I'd like to ask all developers for their opinions. Once we
> reach consensus I'd release the checkstyle file and add the check to the
> build, in the beginning as deactivated profile or non-failing, once all
> issues are fixed activated by default.
>
>
> So here as quick poll, please express your opinion, my preferred option
> is always (1) ;).
>
>
> LocalVariableName:
> Sometimes variable names like 'W1' (Asn1String) or 'Kc' (KcCheckSum) are
> used. However variable names should start with lowercase character. I
> guess this was done to follow names in specifications.
> Poll:
> (1) change variable names
> (2) disable check
Obviously (1)
>
>
> ParameterName:
> Same as above (LocalVariableName)
> Poll:
> (1) change parameter names
> (2) disable check
(1) too
>
>
> MethodName:
> Some test methods contain underscores (e.g.
> DecryptionTest.testDecryptDES_CBC_MD4_1)
> Poll:
> (1) change method names
> (2) disable check
I think it's preferable to diasble the check here. The CIPHEr names use
'_'; it would be less readable to remove them
testDecryptDES_CBC_MD4_1 is way better than testDecryptDesCbcMD41
>
>
> AvoidStarImport:
> There are many usages of '*' imports, like 'import org.ietf.jgss.*;'
> Poll:
> (1) force explicit imports or each class
> (2) allow star imports and disalbe check
(1)
>
>
> AvoidInlineConditionals
> There are 45 inline conditionals (e.g. 'return s != null ?
> s.getVersion() : -1;')
> Poll:
> (1) allow inline condidtionals and disable check
> (2) refactor inline conditionals
(2) : more readable, remove potential error when those conditional
expressions are changed.
>
>
> LineLength:
> Poll:
> (1) allow line length of 120
> (2) force line length of 80
120
>
>
> Whitespace:
> There are about 1100 whitespace violations
> Poll:
> (1) follow Sun/Oracle Java convention (requires reformatting with
> formatter, see below)
> (2) something else
(1)
>
>
> Curly braces:
> There are 89 violations
> Poll:
> (1) follow Sun/Oracle Java convention (requires reformatting with
> formatter, see below)
> (2) something else
(1)
>
>
> Formatter:
> For Eclipse I'd also create a formatter with the following settings:
> * Based on Java Conventions with following changes
> * 4 spaces, no tabs
> * Line length 120 instead of 80
> * No comment formatting (Javadoc, block, line)
> Poll:
> (1) that is fine
> (2) something else
(1)
Thanks Stefan!
Re: Kerby coding style
Posted by Kiran Ayyagari <ka...@apache.org>.
On Sat, Jul 4, 2015 at 10:40 PM, Stefan Seelmann <ma...@stefan-seelmann.de>
wrote:
> Hi all,
>
> I created a basic checkstyle config [1] and fixed some (to me) obvious
> issues. Some more checks make sense which require more changes in the
> code but I'd like to ask all developers for their opinions. Once we
> reach consensus I'd release the checkstyle file and add the check to the
> build, in the beginning as deactivated profile or non-failing, once all
> issues are fixed activated by default.
>
>
> So here as quick poll, please express your opinion, my preferred option
> is always (1) ;).
>
>
+1, likewise, except no preference on line length am fine with either 120
or 80
thanks Stefan
>
> LocalVariableName:
> Sometimes variable names like 'W1' (Asn1String) or 'Kc' (KcCheckSum) are
> used. However variable names should start with lowercase character. I
> guess this was done to follow names in specifications.
> Poll:
> (1) change variable names
> (2) disable check
>
>
> ParameterName:
> Same as above (LocalVariableName)
> Poll:
> (1) change parameter names
> (2) disable check
>
>
> MethodName:
> Some test methods contain underscores (e.g.
> DecryptionTest.testDecryptDES_CBC_MD4_1)
> Poll:
> (1) change method names
> (2) disable check
>
>
> AvoidStarImport:
> There are many usages of '*' imports, like 'import org.ietf.jgss.*;'
> Poll:
> (1) force explicit imports or each class
> (2) allow star imports and disalbe check
>
>
> AvoidInlineConditionals
> There are 45 inline conditionals (e.g. 'return s != null ?
> s.getVersion() : -1;')
> Poll:
> (1) allow inline condidtionals and disable check
> (2) refactor inline conditionals
>
>
> LineLength:
> Poll:
> (1) allow line length of 120
> (2) force line length of 80
>
>
> Whitespace:
> There are about 1100 whitespace violations
> Poll:
> (1) follow Sun/Oracle Java convention (requires reformatting with
> formatter, see below)
> (2) something else
>
>
> Curly braces:
> There are 89 violations
> Poll:
> (1) follow Sun/Oracle Java convention (requires reformatting with
> formatter, see below)
> (2) something else
>
>
> Formatter:
> For Eclipse I'd also create a formatter with the following settings:
> * Based on Java Conventions with following changes
> * 4 spaces, no tabs
> * Line length 120 instead of 80
> * No comment formatting (Javadoc, block, line)
> Poll:
> (1) that is fine
> (2) something else
>
>
> Kind Regards,
> Stefan
>
> [1]
>
> https://svn.apache.org/repos/asf/directory/buildtools/checkstyle-configuration/trunk/src/main/resources/kerby-checks.xml
>
>
--
Kiran Ayyagari
http://keydap.com