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. (I'm still on the docs-on-private-too side of things, but realize that's an extreme position)
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. 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 > >
