+1. I didn't realize checkstyle had the ability to apply policy based on line count. Line count is a fitting heuristic for complexity, so the policy is "complex method need documentation". I would even propose applying this to non-public methods, but I suspect that would be more controversial.
I share Robert's concern for random victims hitting the policy when a method grows from N-1 to N lines. This can easily happen with automatic refactoring + spotless code formatting. For example, renaming a variable to a longer name can introduce new line-breaks. But, I'm think code documentation is valuable enough that it's still worth it. On Tue, Jan 8, 2019 at 4:03 AM Robert Bradshaw <[email protected]> wrote: > With the clarification that we're looking at the intersection of > public + "big", I think this is a great idea. We should make it clear > that this is a lower bar--many private or shorter methods merit > documentation as well (but that's harder to automatically detect). The > one difficulty with a threshold is that it's painful for the person > who does some refactoring or other minor work and turns the (say) > 29-line method into a 30-line one. This is a case where as suppression > annotation (appropriately used) could be useful. > > On Tue, Jan 8, 2019 at 1:49 AM Daniel Oliveira <[email protected]> > wrote: > > > > +1 > > > > I like this idea, especially with the line number requirement. The exact > number of lines is debatable, but you could go as low as 10 lines and that > would exclude any trivial setters and getters. Even better might be if it's > possible to configure checkstyle to ignore this for getters and setters (I > don't know if checkstyle supports this, but I know that other tools are > able to auto-detect getters and setters). > > > > I'm not dead-set against having annotation to suppress the comment, but > it carries the risk that code will be left un-commented because both the > dev and reviewer think it's self-explanatory, and then someone new to the > codebase finds it confusing. > > > > On Mon, Jan 7, 2019 at 11:31 AM Ankur Goenka <[email protected]> wrote: > >> > >> I think it makes sense. > >> Having an annotation to suppress this check for a method/class instead > of adding trivial comment would be useful. > >> > >> On Mon, Jan 7, 2019 at 9:53 AM Ruoyun Huang <[email protected]> wrote: > >>> > >>> Yeah. Agree there is no reason to enforce anything for trivial methods > like setter/getter. > >>> > >>> What I meant is to enforce only for a method that is BOTH 1) public > method 2) has longer than N lines. > >>> > >>> sorry for not making the proposal clear enough in the original > message, it should've better titled "enforce ... on non-trivial public > methods". > >>> > >>> > >>> > >>> On Mon, Jan 7, 2019 at 1:31 AM Robert Bradshaw <[email protected]> > wrote: > >>>> > >>>> IMHO, requiring comments on trivial methods like setters and getters > >>>> is often a net negative, but setting some standard could be useful. > >>>> > >>>> On Mon, Jan 7, 2019 at 7:35 AM Jean-Baptiste Onofré <[email protected]> > wrote: > >>>> > > >>>> > Hi, > >>>> > > >>>> > for the presence of a comment on public method, it's a good idea. > Now, > >>>> > about the number of lines, not sure it's a good idea. I'm thinking > about > >>>> > the getter/setter which are public. Most of the time, the comment is > >>>> > pretty simple (and useless ;)). > >>>> > > >>>> > Regards > >>>> > JB > >>>> > > >>>> > On 07/01/2019 04:35, Ruoyun Huang wrote: > >>>> > > Hi, everyone, > >>>> > > > >>>> > > > >>>> > > We were wondering whether it is a good idea to make checkstyle > >>>> > > enforce public method comments. Our current behavior of JavaDoc > check is: > >>>> > > > >>>> > > 1. > >>>> > > > >>>> > > Missing Class javadoc comment is reported as error. > >>>> > > > >>>> > > 2. > >>>> > > > >>>> > > Method comment missing is explicitly allowed. see [1]. It is > not > >>>> > > even shown as warning. > >>>> > > > >>>> > > 3. > >>>> > > > >>>> > > The actual javadoc target gives warning when certain tags are > >>>> > > missing in javadoc, but not if the whole comment is missing. > >>>> > > > >>>> > > > >>>> > > How about we enforce method comments for **1) public method > and 2) > >>>> > > method that is longer than N lines**. (N=~30 seems a good number, > >>>> > > leading to ~50 violations in current repository). I can find out > the > >>>> > > corresponding contributors to fill in the missing comments, > before we > >>>> > > turning the check fully on. > >>>> > > > >>>> > > > >>>> > > One caveat though is that we might want skip this check on > test code, > >>>> > > but I am not sure yet if our current setup can easily handle > separated > >>>> > > rules for main code versus test code. > >>>> > > > >>>> > > > >>>> > > Is this a good idea? Thoughts and suggestions? > >>>> > > > >>>> > > > >>>> > > [1] > >>>> > > > https://github.com/apache/beam/blame/5ceffb246c0c38ad68dd208e951a1f39c90ef85c/sdks/java/build-tools/src/main/resources/beam/checkstyle.xml#L111 > >>>> > > > >>>> > > > >>>> > > Cheers, > >>>> > > > >>>> > > >>>> > -- > >>>> > Jean-Baptiste Onofré > >>>> > [email protected] > >>>> > http://blog.nanthrax.net > >>>> > Talend - http://www.talend.com > >>> > >>> > >>> > >>> -- > >>> ================ > >>> Ruoyun Huang > >>> > -- Got feedback? tinyurl.com/swegner-feedback
