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 >>
