You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Jonathan Levinson <Jo...@intersystems.com> on 2009/09/27 19:19:18 UTC
Confused about checkstyle use
I've installed the Checkstyle plugin for IDEA and the current code when
scanned by the plugin shows lots of Checkstyle errors.
Here are some errors scanning BlockStackingLayoutManager.java:
Missing package-info.java file (0:0)
Line is longer than 80 characters. (18:0)
First sentence should end with a period (53:0)
Variable 'bpUnit' must be private and have accessor methods. (61:19)
What does it mean to have clean code according to "Checkstyle"?
Is my plugin misconfigured? Is it by default at too strict a setting?
Best Regards,
Jonathan S. Levinson
Senior Software Developer
Object Group
InterSystems
RE: Confused about checkstyle use
Posted by Alexander Kiel <al...@gmx.net>.
Hi Jonathan,
> However, I notice there are still warnings.
>
> BlockStackingLayoutManager.java: 16 items
>
> Missing a Javadoc comment. (58:5)
> 'parentArea' hides a field. (115:47)
> 'parentArea' hides a field. (145:50)
> Method length is 185 lines (max allowed is 150) (372:5)
> Etc.,
At BlockStackingLayoutManager my right side is almost completely
yellow :-) In German, I would say: "Monsterklasse". :-)
> I'm using JetBrains IDEA 8.1.3.
I too.
> BTW, I got Checkstyle to work in IDEA by changing checkstyle-5.0.xml in
> FOP in the following way:
>
> <module name="RegexpHeader">
> <property name="headerFile"
> value="c:/perforce/Users/levinson/fop-trunk/checkstyle.header"/>
Yes, this solution is obvious, but not very suitable as you can't commit
this file with your private path.
Can someone of the older project members point us to some info, why this
"${samedir}" property did not work in IDEA?
Best Regards
Alex
> -----Original Message-----
> From: Alexander Kiel [mailto:alexanderkiel@gmx.net]
> Sent: Sunday, September 27, 2009 4:55 PM
> To: fop-dev@xmlgraphics.apache.org
> Subject: Re: Confused about checkstyle use
>
> Hi Jonathan,
>
> did you use the checkstyle-5.0.xml from FOP or the default SUN profile?
> I'm currently not able to start IDEA, but two days ago as I downloaded
> the plugin, I noticed that the SUn profile was active and I had to
> define the FOP profile. And if you define the FOP profile, you will
> properly notice that the header thing did not work. Its a path inclusion
>
> problem of the header.* file. I did not have a solution for it, I just
> commended it out for now.
>
> Best Regards
> Alex
>
> Jonathan Levinson wrote:
> >
> > I've installed the Checkstyle plugin for IDEA and the current code
> > when scanned by the plugin shows lots of Checkstyle errors.
> >
> > Here are some errors scanning BlockStackingLayoutManager.java:
> >
> > Missing package-info.java file (0:0)
> >
> > Line is longer than 80 characters. (18:0)
> >
> > First sentence should end with a period (53:0)
> >
> > Variable 'bpUnit' must be private and have accessor methods. (61:19)
> >
> > What does it mean to have clean code according to "Checkstyle"?
> >
> > Is my plugin misconfigured? Is it by default at too strict a setting?
> >
> > Best Regards,
> >
> > Jonathan S. Levinson
> >
> > Senior Software Developer
> >
> > Object Group
> >
> > InterSystems
> >
>
>
Re: Confused about checkstyle use
Posted by Alexander Kiel <al...@gmx.net>.
Hi Vincent,
in Intellij Idea, I have also annoying yellow marks in my code. So if
the common policy is to not violate any warning, I won't do that.
Best Regards
Alex
-
e-mail: alexanderkiel@gmx.net
web: www.alexanderkiel.net
On Thu, 2009-10-01 at 10:41 +0100, Vincent Hennebert wrote:
> Hi Alexander,
>
> Alexander Kiel wrote:
> > Hi Vincent,
> >
> >> Should the rule be disabled because of that? Having proper javadoc on at
> >> least public methods is very important. OTOH, this is actually not
> >> something Checkstyle can verify. How many methods in the code base have
> >> totally useless comments that are there just to avoid a Checkstyle
> >> warning...
> >>
> >> I think I’d prefer to keep the rule, but wouldn’t veto its removal.
> >
> > I don't vote for removal too, I only vote for the right to violate it in
> > cases one can't add any useful information in the comment.
>
> Hmmm, I think that once we’ve agreed on a Checkstyle config we really
> want to follow, we won’t accept any warning at all. It was my intent to
> propose that anyway. I think it’s more annoying to have little yellow
> exclamation marks attached to every file that contains Checkstyle
> warnings (in Eclipse, at least), than have dull javadoc comments.
>
>
> Vincent
>
>
Re: Confused about checkstyle use
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Alexander,
Alexander Kiel wrote:
> Hi Vincent,
>
>> Should the rule be disabled because of that? Having proper javadoc on at
>> least public methods is very important. OTOH, this is actually not
>> something Checkstyle can verify. How many methods in the code base have
>> totally useless comments that are there just to avoid a Checkstyle
>> warning...
>>
>> I think I’d prefer to keep the rule, but wouldn’t veto its removal.
>
> I don't vote for removal too, I only vote for the right to violate it in
> cases one can't add any useful information in the comment.
Hmmm, I think that once we’ve agreed on a Checkstyle config we really
want to follow, we won’t accept any warning at all. It was my intent to
propose that anyway. I think it’s more annoying to have little yellow
exclamation marks attached to every file that contains Checkstyle
warnings (in Eclipse, at least), than have dull javadoc comments.
Vincent
Re: Confused about checkstyle use
Posted by Alexander Kiel <al...@gmx.net>.
Hi Vincent,
> Should the rule be disabled because of that? Having proper javadoc on at
> least public methods is very important. OTOH, this is actually not
> something Checkstyle can verify. How many methods in the code base have
> totally useless comments that are there just to avoid a Checkstyle
> warning...
>
> I think I’d prefer to keep the rule, but wouldn’t veto its removal.
I don't vote for removal too, I only vote for the right to violate it in
cases one can't add any useful information in the comment.
Best Regards
Alex
Re: Confused about checkstyle use
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Alexander,
Alexander Kiel wrote:
> Hi Max,
>
> First, I will respect every code style of FOP. Its just a matter of
> discussion.
>
>>> Really? That means commenting every public method even simple Getters
>>> and Setters?
>> Yes. Simple Getter and Setters are the only place where you can
>> publicly document private variables. (in most cases, comment in the
>> getter and link from the setter)
>
> Yes thats right. But is this Javadoc better than no Javadoc?
>
> public class Person {
>
> /**
> * Returns the first name of this person.
> *
> * @returns the first name of this person.
> */
> public String getFirstName() {
> return firstName;
> }
> }
Except in the simplest cases like that one, there is always a bit of
additional information that can be added about the variable or its
usage.
>>> Commenting equals(), hashCode() and toString()? I think,
>>> this would be only clutter.
>> /** {@inheritDoc} */
>
> In my eyes this is enough clutter. I saw classes in FOP with maybe 10
> methods using this /** {@inheritDoc} */. It just distracts the eye from
> ready the actual method name. And it adds absolutely no information for
> the source code reader.
That one is indeed there only to make Checkstyle happy. The Javadoc tool
is able to retrieve by itself the javadoc from the redefined method
(Eclipse as well). I wish Checkstyle could do that too. We will be able
to partially solve that when switching to Java 1.5, by using the
@Override annotation.
Should the rule be disabled because of that? Having proper javadoc on at
least public methods is very important. OTOH, this is actually not
something Checkstyle can verify. How many methods in the code base have
totally useless comments that are there just to avoid a Checkstyle
warning...
I think I’d prefer to keep the rule, but wouldn’t veto its removal.
>> would do the trick on those, UNLESS they implement something which is
>> unexpected (such as the equals methods I recently renamed which did
>> not implement equals) or special (a toString which creates a
>> guaranteed parsable result for example)
>
> Hmmm. A equals method shouldn't do anything unexpected. But your
> toString() example is a good one. If such standard methods do something
> more as the comment in Object says, that a comment is useful.
>
> I think it's the same as on simple public methods like the getter from
> above. If your comment doesn't say anything more than the method name
> says already, I don't want to read it.
>
> Best Regards
> Alex
Vincent
Re: Confused about checkstyle use
Posted by Alexander Kiel <al...@gmx.net>.
Hi Max,
First, I will respect every code style of FOP. Its just a matter of
discussion.
> > Really? That means commenting every public method even simple Getters
> > and Setters?
>
> Yes. Simple Getter and Setters are the only place where you can
> publicly document private variables. (in most cases, comment in the
> getter and link from the setter)
Yes thats right. But is this Javadoc better than no Javadoc?
public class Person {
/**
* Returns the first name of this person.
*
* @returns the first name of this person.
*/
public String getFirstName() {
return firstName;
}
}
> > Commenting equals(), hashCode() and toString()? I think,
> > this would be only clutter.
>
> /** {@inheritDoc} */
In my eyes this is enough clutter. I saw classes in FOP with maybe 10
methods using this /** {@inheritDoc} */. It just distracts the eye from
ready the actual method name. And it adds absolutely no information for
the source code reader.
> would do the trick on those, UNLESS they implement something which is
> unexpected (such as the equals methods I recently renamed which did
> not implement equals) or special (a toString which creates a
> guaranteed parsable result for example)
Hmmm. A equals method shouldn't do anything unexpected. But your
toString() example is a good one. If such standard methods do something
more as the comment in Object says, that a comment is useful.
I think it's the same as on simple public methods like the getter from
above. If your comment doesn't say anything more than the method name
says already, I don't want to read it.
Best Regards
Alex
Re: Confused about checkstyle use
Posted by Max Berger <ma...@berger.name>.
Alex,
2009/9/28 Alexander Kiel <al...@gmx.net>:
> Hi Vincent,
>
>> However, new committed code is not supposed to break any rule, neither
>> warnings nor errors.
>
> Really? That means commenting every public method even simple Getters
> and Setters?
Yes. Simple Getter and Setters are the only place where you can
publicly document private variables. (in most cases, comment in the
getter and link from the setter)
> Commenting equals(), hashCode() and toString()? I think,
> this would be only clutter.
/** {@inheritDoc} */
would do the trick on those, UNLESS they implement something which is
unexpected (such as the equals methods I recently renamed which did
not implement equals) or special (a toString which creates a
guaranteed parsable result for example)
> Best Regards
> Alex
hth
Max
Re: Confused about checkstyle use
Posted by Alexander Kiel <al...@gmx.net>.
Hi Vincent,
> However, new committed code is not supposed to break any rule, neither
> warnings nor errors.
Really? That means commenting every public method even simple Getters
and Setters? Commenting equals(), hashCode() and toString()? I think,
this would be only clutter.
Best Regards
Alex
Re: Confused about checkstyle use
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Guys,
Maybe a bit late, but you may find the following page interesting:
http://wiki.apache.org/xmlgraphics-fop/FOPIntelliJSetup
Feel free to make any additions or corrections to this document.
As to the use of checkstyle: I think the rules were set up long after
the project was created. So the code that was already existing didn’t
follow all of them. And modifying it accordingly isn’t a straightforward
nor enjoyable task...
However, new committed code is not supposed to break any rule, neither
warnings nor errors.
HTH,
Vincent
Jonathan Levinson wrote:
> Thanks to your advice (and my finding the checkstyle configurator in
> Idea) I'm now using checkstyle-5.0.xml from FOP. Thank you very much!
>
> However, I notice there are still warnings.
>
> BlockStackingLayoutManager.java: 16 items
>
> Missing a Javadoc comment. (58:5)
> 'parentArea' hides a field. (115:47)
> 'parentArea' hides a field. (145:50)
> Method length is 185 lines (max allowed is 150) (372:5)
> Etc.,
>
> I'm using JetBrains IDEA 8.1.3.
>
> Is the rule we ignore warnings and only look for errors?
>
> BTW, I got Checkstyle to work in IDEA by changing checkstyle-5.0.xml in
> FOP in the following way:
>
> <module name="RegexpHeader">
> <property name="headerFile"
> value="c:/perforce/Users/levinson/fop-trunk/checkstyle.header"/>
>
> Thanks again for your help!,
> Jonathan S. Levinson
> Senior Software Developer
> Object Group
> InterSystems
>
>
> -----Original Message-----
> From: Alexander Kiel [mailto:alexanderkiel@gmx.net]
> Sent: Sunday, September 27, 2009 4:55 PM
> To: fop-dev@xmlgraphics.apache.org
> Subject: Re: Confused about checkstyle use
>
> Hi Jonathan,
>
> did you use the checkstyle-5.0.xml from FOP or the default SUN profile?
> I'm currently not able to start IDEA, but two days ago as I downloaded
> the plugin, I noticed that the SUn profile was active and I had to
> define the FOP profile. And if you define the FOP profile, you will
> properly notice that the header thing did not work. Its a path inclusion
>
> problem of the header.* file. I did not have a solution for it, I just
> commended it out for now.
>
> Best Regards
> Alex
>
> Jonathan Levinson wrote:
>> I've installed the Checkstyle plugin for IDEA and the current code
>> when scanned by the plugin shows lots of Checkstyle errors.
>>
>> Here are some errors scanning BlockStackingLayoutManager.java:
>>
>> Missing package-info.java file (0:0)
>>
>> Line is longer than 80 characters. (18:0)
>>
>> First sentence should end with a period (53:0)
>>
>> Variable 'bpUnit' must be private and have accessor methods. (61:19)
>>
>> What does it mean to have clean code according to "Checkstyle"?
>>
>> Is my plugin misconfigured? Is it by default at too strict a setting?
>>
>> Best Regards,
>>
>> Jonathan S. Levinson
>>
>> Senior Software Developer
>>
>> Object Group
>>
>> InterSystems
>>
>
RE: Confused about checkstyle use
Posted by Jonathan Levinson <Jo...@intersystems.com>.
Thanks to your advice (and my finding the checkstyle configurator in
Idea) I'm now using checkstyle-5.0.xml from FOP. Thank you very much!
However, I notice there are still warnings.
BlockStackingLayoutManager.java: 16 items
Missing a Javadoc comment. (58:5)
'parentArea' hides a field. (115:47)
'parentArea' hides a field. (145:50)
Method length is 185 lines (max allowed is 150) (372:5)
Etc.,
I'm using JetBrains IDEA 8.1.3.
Is the rule we ignore warnings and only look for errors?
BTW, I got Checkstyle to work in IDEA by changing checkstyle-5.0.xml in
FOP in the following way:
<module name="RegexpHeader">
<property name="headerFile"
value="c:/perforce/Users/levinson/fop-trunk/checkstyle.header"/>
Thanks again for your help!,
Jonathan S. Levinson
Senior Software Developer
Object Group
InterSystems
-----Original Message-----
From: Alexander Kiel [mailto:alexanderkiel@gmx.net]
Sent: Sunday, September 27, 2009 4:55 PM
To: fop-dev@xmlgraphics.apache.org
Subject: Re: Confused about checkstyle use
Hi Jonathan,
did you use the checkstyle-5.0.xml from FOP or the default SUN profile?
I'm currently not able to start IDEA, but two days ago as I downloaded
the plugin, I noticed that the SUn profile was active and I had to
define the FOP profile. And if you define the FOP profile, you will
properly notice that the header thing did not work. Its a path inclusion
problem of the header.* file. I did not have a solution for it, I just
commended it out for now.
Best Regards
Alex
Jonathan Levinson wrote:
>
> I've installed the Checkstyle plugin for IDEA and the current code
> when scanned by the plugin shows lots of Checkstyle errors.
>
> Here are some errors scanning BlockStackingLayoutManager.java:
>
> Missing package-info.java file (0:0)
>
> Line is longer than 80 characters. (18:0)
>
> First sentence should end with a period (53:0)
>
> Variable 'bpUnit' must be private and have accessor methods. (61:19)
>
> What does it mean to have clean code according to "Checkstyle"?
>
> Is my plugin misconfigured? Is it by default at too strict a setting?
>
> Best Regards,
>
> Jonathan S. Levinson
>
> Senior Software Developer
>
> Object Group
>
> InterSystems
>
Re: Confused about checkstyle use
Posted by Alexander Kiel <al...@gmx.net>.
Hi Jonathan,
did you use the checkstyle-5.0.xml from FOP or the default SUN profile?
I'm currently not able to start IDEA, but two days ago as I downloaded
the plugin, I noticed that the SUn profile was active and I had to
define the FOP profile. And if you define the FOP profile, you will
properly notice that the header thing did not work. Its a path inclusion
problem of the header.* file. I did not have a solution for it, I just
commended it out for now.
Best Regards
Alex
Jonathan Levinson wrote:
>
> I’ve installed the Checkstyle plugin for IDEA and the current code
> when scanned by the plugin shows lots of Checkstyle errors.
>
> Here are some errors scanning BlockStackingLayoutManager.java:
>
> Missing package-info.java file (0:0)
>
> Line is longer than 80 characters. (18:0)
>
> First sentence should end with a period (53:0)
>
> Variable ‘bpUnit’ must be private and have accessor methods. (61:19)
>
> What does it mean to have clean code according to “Checkstyle”?
>
> Is my plugin misconfigured? Is it by default at too strict a setting?
>
> Best Regards,
>
> Jonathan S. Levinson
>
> Senior Software Developer
>
> Object Group
>
> InterSystems
>