Half agree, half disagree Disagree: best practice is to make your constructors private and have distinct static methods for various modes of instantiation, which will then require Javadoc.
Agree: once they are private no javadoc is needed :-) and there's often only one "most general" constructor that all the static methods use in different ways Kenn On Mon, Jan 28, 2019 at 5:03 PM Ruoyun Huang <ruo...@google.com> wrote: > 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 <re...@google.com> 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 <ruo...@google.com> 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 <ajam...@google.com> 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 <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 >>>>> >>>>> >>> >>> -- >>> ================ >>> Ruoyun Huang >>> >>> > > -- > ================ > Ruoyun Huang > >
