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 <ruo...@google.com> 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 <k...@apache.org> 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 <ruo...@google.com> 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 <ruo...@google.com> 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 <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 >>>> >>>> >>> >>> -- >>> ================ >>> Ruoyun Huang >>> >>> > > -- > ================ > Ruoyun Huang > >
