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
