created a PR: https://github.com/apache/beam/pull/7454
Note instead of having separated checkstyle specs for Main versus Test, this PR simply uses suppression to turn off JavaDocComment for test files. If this PR draft looks good, then next step I will commit another change that: 1) throw error on violations (now just warning to keep PR green). 2) List all the violations explicitly in a suppression list, and let area contributors/owners address and chop things off the list over time. Not ideal and quite some manual work, if there is a better way, please let me know. On Wed, Jan 9, 2019 at 7:29 AM Robert Bradshaw <rober...@google.com> wrote: > On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <k...@apache.org> wrote: > > > > I think @Internal would be a reasonable annotation to exempt from > documentation, as that means it is explicitly *not* part of the actual > public API, as Ismaël alluded to. > > We'll probably want a distinct annotation from that. Forced comments, > especially forced-by-an-impartial-metric ones, are often lower > quality. This is the kind of signal that would be useful to surface to > a reviewer who could then (jointly) make the call rather than it being > a binary failure/success. > > > (I'm still on the docs-on-private-too side of things, but realize that's > an extreme position) > > +1 to docs on private things as well, though maybe with not as high > priority :). > > > It is a shame that we chose blacklist (via @Internal) instead of > whitelist (via e.g. @Public) for what constitutes an actual supported > public method. > > Probably better than having to re-train others that public doesn't > really mean public unless it has a @Public on it. It's harder to > "unknowingly" use an @Internal API. > > > > Kenn > > > > On Tue, Jan 8, 2019 at 1:46 PM Ruoyun Huang <ruo...@google.com> wrote: > >> > >> To Ismael's question: When applying such a check (i.e. public method > with >30 Loc), our code base shows in total 115 violations. > >> > >> Thanks for the feedback everyone. As some of you mentioned already, > suppress warning is always available whenever contributor/reviewer feels > appropriate, instead of been forced to put in low quality comments. This > check is more about to help us avoid human errors, in those cases we do > want to add meaningful javadocs. > >> > >> With 5 +1s so far. I will put together a PR draft. A bit research is > still needed regarding the best practise to apply check to Main/Test in a > different way. If anyone has experience on it, please share it with me. > >> > >> > >> > >> > >> > >> On Tue, Jan 8, 2019 at 8:19 AM Ismaël Mejía <ieme...@gmail.com> wrote: > >>> > >>> -0 > >>> > >>> Same comments than Robert I am particularly worried on how this affect > >>> contributors in particular casual ones. Even if the intended idea is > >>> good I am also worried that people just write poor comments to get rid > >>> of the annoyance. > >>> > >>> Have you already estimated how hard is the current codebase impacted? > >>> Or how many methods will be needed to document before this gets in > >>> place? > >>> > >>> I wouldn't be surprised if many runners or internal parts of the > >>> codebase lack comments on public methods considering that the 'public > >>> API' of must runners 'is not' the public methods but the specific > >>> PipelineOptions, and for some methods (even longer ones) such comments > >>> may be trivial. > >>> > >>> On Tue, Jan 8, 2019 at 5:16 PM Kenneth Knowles <k...@apache.org> > wrote: > >>> > > >>> > +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 > >> > >> > >> > >> -- > >> ================ > >> Ruoyun Huang > >> > -- ================ Ruoyun Huang
