The problem is existing code. I find that if I touch existing code that has a constructor, I am then forced to add a Javadoc on the constructor. Usually this Javadoc is "Constructs XX object."
On Mon, Jan 28, 2019 at 5:07 PM Kenneth Knowles <[email protected]> wrote: > 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 <[email protected]> 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 <[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 >> >>
