+1 I even thought this was already on (at some point). On Tue, Jan 8, 2019 at 8:01 AM Scott Wegner <sc...@apache.org> wrote:
> I would even propose applying this to non-public methods, but I suspect > that would be more controversial. > I also would support this. It will improve code quality as well. Often missing doc means something is not well thought-out. It often also indicates a misguided attempt to "share code" without sharing a clear concept. 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. > Another perspective is that someone is getting away with missing documentation at N-1. Seems OK. But maybe just allowMissingPropertyJavadoc (from http://checkstyle.sourceforge.net/config_javadoc.html#JavadocMethod)? We can also configure allowedAnnotations but if you are going to go through the trouble of annotating something, a javadoc comment is just as easy. I want to caveat this: I am strongly opposed to any requirements on the contents of the javadoc, which is almost all the time redundant bloat if the description is at all adequate. Kenn > On Tue, Jan 8, 2019 at 4:03 AM Robert Bradshaw <rober...@google.com> > 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 <danolive...@google.com> >> 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 <goe...@google.com> 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 <ruo...@google.com> 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 <rober...@google.com> >> 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é <j...@nanthrax.net> >> 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é >> >>>> > jbono...@apache.org >> >>>> > http://blog.nanthrax.net >> >>>> > Talend - http://www.talend.com >> >>> >> >>> >> >>> >> >>> -- >> >>> ================ >> >>> Ruoyun Huang >> >>> >> > > > -- > > > > > Got feedback? tinyurl.com/swegner-feedback >
