Fair point. Looking at JavaDocMethod spec [1], unfortunately there is no properties available for this tweak.
Let me dig a bit more to see whether this can be done via suppression. [1] http://checkstyle.sourceforge.net/config_javadoc.html#JavadocMethod On Mon, Jan 28, 2019 at 4:36 PM Reuven Lax <[email protected]> wrote: > This appears to be forcing us to set javadoc on constructors as well, > which is usually pointless. Can we exclude constructor methods from this > check? > > On Wed, Jan 23, 2019 at 5:40 PM Ruoyun Huang <[email protected]> wrote: > >> Our recent change is on "JavaDocMethod", which not turned on yet. Not >> relevant to this error here. >> >> The one throws error is "javaDocType". it has been there for a while >> <https://github.com/apache/beam/blame/master/sdks/java/build-tools/src/main/resources/beam/checkstyle.xml#L156>, >> which is for public class javadoc missing. Yeah, I am curious as well why >> preCommit didn't catch this one. >> >> >> >> On Wed, Jan 23, 2019 at 5:28 PM Alex Amato <[email protected]> wrote: >> >>> Did their happen to be a short time window where some missing Javadoc >>> comments went in? I am now seeing precommit fail due to code I didn't >>> modify. >>> >>> >>> https://scans.gradle.com/s/nwgb7xegklwqo/console-log?task=:beam-runners-direct-java:checkstyleMain >>> >>> >>> >>> On Wed, Jan 23, 2019 at 2:34 PM Ruoyun Huang <[email protected]> wrote: >>> >>>> Trying to understand your suggestion. By saying "break that >>>> dependency", do you mean moving checkstyle out of Java PreCommit? >>>> >>>> currently we do have checkstyle as part of ":check". It seems to me >>>> "check" does minimal amount of essential works (correct me If I am wrong), >>>> much less than what PreCommit does. >>>> >>>> On Wed, Jan 23, 2019 at 12:20 PM Kenneth Knowles <[email protected]> >>>> wrote: >>>> >>>>> It is always a bummer when the Java PreCommit fails due to style >>>>> checking. Can we get this to run separately and quicker? I notice it >>>>> depends on compileJava. I cannot remember why that is, but I recall it is >>>>> a >>>>> legitimate reason. Nonetheless, can we break that dependency somehow? >>>>> >>>>> Kenn >>>>> >>>>> On Wed, Jan 16, 2019 at 6:42 PM Ruoyun Huang <[email protected]> >>>>> wrote: >>>>> >>>>>> Hi, everyone, >>>>>> >>>>>> >>>>>> To make sure we move forward to a clean state where we catch >>>>>> violations in any new PR, we created this change: >>>>>> https://github.com/apache/beam/pull/7532 >>>>>> >>>>>> This PR makes checkstyle to report error on missing javadocs. For >>>>>> existing violations, we explicitly added them as suppression rules, down >>>>>> to >>>>>> which line in the code. >>>>>> >>>>>> The caveat is, once this PR is merged, whoever make update to any >>>>>> file in the list, will very likely have to fix the existing violation for >>>>>> that file. :-) Hope this sounds like a reasonable idea to move forward. >>>>>> >>>>>> In the meanwhile, I will try to address the items in the list (if I >>>>>> can). And over time, I will get back to this and remove those >>>>>> suppressions >>>>>> no longer needed (created JIRA-6446 for tracking purpose), until all >>>>>> of them are fixed. >>>>>> >>>>>> On Wed, Jan 9, 2019 at 10:57 PM Ruoyun Huang <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> 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 <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>> wrote: >>>>>>>> >>> > >>>>>>>> >>> > +1 I even thought this was already on (at some point). >>>>>>>> >>> > >>>>>>>> >>> > On Tue, Jan 8, 2019 at 8:01 AM Scott Wegner <[email protected]> >>>>>>>> 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 < >>>>>>>> [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 >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> -- >>>>>>>> >> ================ >>>>>>>> >> Ruoyun Huang >>>>>>>> >> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> ================ >>>>>>> Ruoyun Huang >>>>>>> >>>>>>> >>>>>> >>>>>> -- >>>>>> ================ >>>>>> Ruoyun Huang >>>>>> >>>>>> >>>> >>>> -- >>>> ================ >>>> Ruoyun Huang >>>> >>>> >> >> -- >> ================ >> Ruoyun Huang >> >> -- ================ Ruoyun Huang
