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
