Re: Confused about checkstyle use
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: alexanderk...@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 > > signature.asc Description: This is a digitally signed message part
Re: Confused about checkstyle use
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
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 signature.asc Description: This is a digitally signed message part
Re: Confused about checkstyle use
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
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 signature.asc Description: This is a digitally signed message part
Re: Confused about checkstyle use
Alex, 2009/9/28 Alexander Kiel : > 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
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: > > > 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:alexanderk...@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 > > > > signature.asc Description: This is a digitally signed message part
Re: Confused about checkstyle use
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 signature.asc Description: This is a digitally signed message part
Re: Confused about checkstyle use
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: > > > 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:alexanderk...@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
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: Thanks again for your help!, Jonathan S. Levinson Senior Software Developer Object Group InterSystems -Original Message- From: Alexander Kiel [mailto:alexanderk...@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
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
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