You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Ruoyun Huang <ru...@google.com> on 2019/01/07 03:35:15 UTC
Enforce javadoc comments in public methods?
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,
Re: Enforce javadoc comments in public methods?
Posted by Kenneth Knowles <kl...@google.com>.
It sounds like there's widespread practices in our codebase that this new
restriction might improve.
Instead of /** Constructs XX object */ you could add a @deprecated and/or a
TODO to migrate the code.
Kenn
On Mon, Jan 28, 2019 at 11:46 PM Reuven Lax <re...@google.com> wrote:
> 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 <ke...@apache.org> 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 <ru...@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 <ru...@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 <aj...@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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <
>>>>>>>>>> robertwb@google.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <
>>>>>>>>>>> iemejia@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 <
>>>>>>>>>>> kenn@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 <
>>>>>>>>>>> scott@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 <
>>>>>>>>>>> robertwb@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 <
>>>>>>>>>>> danoliveira@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 <
>>>>>>>>>>> goenka@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 <
>>>>>>>>>>> ruoyun@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 <
>>>>>>>>>>> robertwb@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é
>>>>>>>>>>> <jb...@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é
>>>>>>>>>>> >>> >>> >>>> > jbonofre@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
>>>
>>>
Re: Enforce javadoc comments in public methods?
Posted by Reuven Lax <re...@google.com>.
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 <ke...@apache.org> 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 <ru...@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 <ru...@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 <aj...@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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <
>>>>>>>>> robertwb@google.com> wrote:
>>>>>>>>>
>>>>>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <
>>>>>>>>>> kenn@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 <
>>>>>>>>>> scott@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 <
>>>>>>>>>> robertwb@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 <
>>>>>>>>>> danoliveira@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 <
>>>>>>>>>> goenka@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 <
>>>>>>>>>> ruoyun@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 <
>>>>>>>>>> robertwb@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é <
>>>>>>>>>> jb@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é
>>>>>>>>>> >>> >>> >>>> > jbonofre@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
>>
>>
Re: Enforce javadoc comments in public methods?
Posted by Kenneth Knowles <ke...@apache.org>.
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 <ru...@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 <ru...@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 <aj...@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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <ro...@google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <
>>>>>>>>> kenn@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 <
>>>>>>>>> scott@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 <
>>>>>>>>> robertwb@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 <
>>>>>>>>> danoliveira@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 <
>>>>>>>>> goenka@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 <
>>>>>>>>> ruoyun@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 <
>>>>>>>>> robertwb@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é <
>>>>>>>>> jb@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é
>>>>>>>>> >>> >>> >>>> > jbonofre@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
>
>
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <ru...@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 <aj...@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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <ro...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>>>>>>>> robertwb@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 <
>>>>>>>> danoliveira@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 <
>>>>>>>> goenka@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 <
>>>>>>>> ruoyun@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 <
>>>>>>>> robertwb@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é <
>>>>>>>> jb@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é
>>>>>>>> >>> >>> >>>> > jbonofre@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
Re: Enforce javadoc comments in public methods?
Posted by Reuven Lax <re...@google.com>.
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 <ru...@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 <aj...@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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <ro...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>>>>>>> robertwb@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 <
>>>>>>> danoliveira@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 <
>>>>>>> goenka@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 <
>>>>>>> ruoyun@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 <
>>>>>>> robertwb@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é <
>>>>>>> jb@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é
>>>>>>> >>> >>> >>>> > jbonofre@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
>
>
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <aj...@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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <ro...@google.com>
>>>>> wrote:
>>>>>
>>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>>>>>> robertwb@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 <
>>>>>> danoliveira@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 <
>>>>>> goenka@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 <
>>>>>> ruoyun@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 <
>>>>>> robertwb@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é <
>>>>>> jb@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é
>>>>>> >>> >>> >>>> > jbonofre@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
Re: Enforce javadoc comments in public methods?
Posted by Alex Amato <aj...@google.com>.
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 <ru...@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 <ke...@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 <ru...@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 <ru...@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 <ro...@google.com>
>>>> wrote:
>>>>
>>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>>>>> robertwb@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 <
>>>>> danoliveira@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 <
>>>>> goenka@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 <
>>>>> ruoyun@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 <
>>>>> robertwb@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é <
>>>>> jb@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é
>>>>> >>> >>> >>>> > jbonofre@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
>
>
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <ke...@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 <ru...@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 <ru...@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 <ro...@google.com>
>>> wrote:
>>>
>>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>>>> robertwb@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 <
>>>> danoliveira@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 <
>>>> goenka@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 <
>>>> ruoyun@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 <
>>>> robertwb@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é <
>>>> jb@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é
>>>> >>> >>> >>>> > jbonofre@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
Re: Enforce javadoc comments in public methods?
Posted by Kenneth Knowles <ke...@apache.org>.
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 <ru...@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 <ru...@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 <ro...@google.com>
>> wrote:
>>
>>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>>> robertwb@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 <
>>> danoliveira@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 <
>>> goenka@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 <
>>> ruoyun@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 <
>>> robertwb@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é <
>>> jb@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é
>>> >>> >>> >>>> > jbonofre@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
>
>
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <ru...@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 <ro...@google.com>
> wrote:
>
>> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <
>> robertwb@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 <
>> danoliveira@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 <go...@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 <ru...@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 <
>> robertwb@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é <
>> jb@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é
>> >>> >>> >>>> > jbonofre@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
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <ro...@google.com> wrote:
> On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <ro...@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 <
> danoliveira@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 <go...@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 <ru...@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 <
> robertwb@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é <
> jb@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é
> >>> >>> >>>> > jbonofre@apache.org
> >>> >>> >>>> > http://blog.nanthrax.net
> >>> >>> >>>> > Talend - http://www.talend.com
> >>> >>> >>>
> >>> >>> >>>
> >>> >>> >>>
> >>> >>> >>> --
> >>> >>> >>> ================
> >>> >>> >>> Ruoyun Huang
> >>> >>> >>>
> >>> >>
> >>> >>
> >>> >>
> >>> >> --
> >>> >>
> >>> >>
> >>> >>
> >>> >>
> >>> >> Got feedback? tinyurl.com/swegner-feedback
> >>
> >>
> >>
> >> --
> >> ================
> >> Ruoyun Huang
> >>
>
--
================
Ruoyun Huang
Re: Enforce javadoc comments in public methods?
Posted by Robert Bradshaw <ro...@google.com>.
On Tue, Jan 8, 2019 at 11:15 PM Kenneth Knowles <ke...@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 <ru...@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 <ie...@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 <ke...@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 <ro...@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 <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>>> >>> >>>> > jbonofre@apache.org
>>> >>> >>>> > http://blog.nanthrax.net
>>> >>> >>>> > Talend - http://www.talend.com
>>> >>> >>>
>>> >>> >>>
>>> >>> >>>
>>> >>> >>> --
>>> >>> >>> ================
>>> >>> >>> Ruoyun Huang
>>> >>> >>>
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >>
>>> >>
>>> >>
>>> >>
>>> >> Got feedback? tinyurl.com/swegner-feedback
>>
>>
>>
>> --
>> ================
>> Ruoyun Huang
>>
Re: Enforce javadoc comments in public methods?
Posted by Kenneth Knowles <ke...@apache.org>.
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. (I'm still on the docs-on-private-too
side of things, but realize that's an extreme position)
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.
Kenn
On Tue, Jan 8, 2019 at 1:46 PM Ruoyun Huang <ru...@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 <ie...@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 <ke...@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 <ro...@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 <
>> danoliveira@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 <go...@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 <ru...@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 <
>> robertwb@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é <
>> jb@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é
>> >>> >>>> > jbonofre@apache.org
>> >>> >>>> > http://blog.nanthrax.net
>> >>> >>>> > Talend - http://www.talend.com
>> >>> >>>
>> >>> >>>
>> >>> >>>
>> >>> >>> --
>> >>> >>> ================
>> >>> >>> Ruoyun Huang
>> >>> >>>
>> >>
>> >>
>> >>
>> >> --
>> >>
>> >>
>> >>
>> >>
>> >> Got feedback? tinyurl.com/swegner-feedback
>>
>
>
> --
> ================
> Ruoyun Huang
>
>
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <ie...@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 <ke...@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 <ro...@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 <da...@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 <go...@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 <ru...@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 <
> robertwb@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é <
> jb@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é
> >>> >>>> > jbonofre@apache.org
> >>> >>>> > http://blog.nanthrax.net
> >>> >>>> > Talend - http://www.talend.com
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> --
> >>> >>> ================
> >>> >>> Ruoyun Huang
> >>> >>>
> >>
> >>
> >>
> >> --
> >>
> >>
> >>
> >>
> >> Got feedback? tinyurl.com/swegner-feedback
>
--
================
Ruoyun Huang
Re: Enforce javadoc comments in public methods?
Posted by Ismaël Mejía <ie...@gmail.com>.
-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 <ke...@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 <ro...@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 <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>>> >>>> > jbonofre@apache.org
>>> >>>> > http://blog.nanthrax.net
>>> >>>> > Talend - http://www.talend.com
>>> >>>
>>> >>>
>>> >>>
>>> >>> --
>>> >>> ================
>>> >>> Ruoyun Huang
>>> >>>
>>
>>
>>
>> --
>>
>>
>>
>>
>> Got feedback? tinyurl.com/swegner-feedback
Re: Enforce javadoc comments in public methods?
Posted by Kenneth Knowles <ke...@apache.org>.
+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 <ro...@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 <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>> >>>> > jbonofre@apache.org
>> >>>> > http://blog.nanthrax.net
>> >>>> > Talend - http://www.talend.com
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> ================
>> >>> Ruoyun Huang
>> >>>
>>
>
>
> --
>
>
>
>
> Got feedback? tinyurl.com/swegner-feedback
>
Re: Enforce javadoc comments in public methods?
Posted by Scott Wegner <sc...@apache.org>.
+1. I didn't realize checkstyle had the ability to apply policy based on
line count. Line count is a fitting heuristic for complexity, so the policy
is "complex method need documentation". I would even propose applying this
to non-public methods, but I suspect that would be more controversial.
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.
On Tue, Jan 8, 2019 at 4:03 AM Robert Bradshaw <ro...@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 <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
> >>>> > jbonofre@apache.org
> >>>> > http://blog.nanthrax.net
> >>>> > Talend - http://www.talend.com
> >>>
> >>>
> >>>
> >>> --
> >>> ================
> >>> Ruoyun Huang
> >>>
>
--
Got feedback? tinyurl.com/swegner-feedback
Re: Enforce javadoc comments in public methods?
Posted by Robert Bradshaw <ro...@google.com>.
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 <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>>>> > jbonofre@apache.org
>>>> > http://blog.nanthrax.net
>>>> > Talend - http://www.talend.com
>>>
>>>
>>>
>>> --
>>> ================
>>> Ruoyun Huang
>>>
Re: Enforce javadoc comments in public methods?
Posted by Connell O'Callaghan <co...@google.com>.
+1
On Mon, Jan 7, 2019 at 5:22 PM Udi Meiri <eh...@google.com> wrote:
> +1
>
> On Mon, Jan 7, 2019 at 4:49 PM Daniel Oliveira <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>>>>> > jbonofre@apache.org
>>>>> > http://blog.nanthrax.net
>>>>> > Talend - http://www.talend.com
>>>>>
>>>>
>>>>
>>>> --
>>>> ================
>>>> Ruoyun Huang
>>>>
>>>>
Re: Enforce javadoc comments in public methods?
Posted by Udi Meiri <eh...@google.com>.
+1
On Mon, Jan 7, 2019 at 4:49 PM Daniel Oliveira <da...@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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>>>> > jbonofre@apache.org
>>>> > http://blog.nanthrax.net
>>>> > Talend - http://www.talend.com
>>>>
>>>
>>>
>>> --
>>> ================
>>> Ruoyun Huang
>>>
>>>
Re: Enforce javadoc comments in public methods?
Posted by Daniel Oliveira <da...@google.com>.
+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 <go...@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 <ru...@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 <ro...@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é <jb...@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é
>>> > jbonofre@apache.org
>>> > http://blog.nanthrax.net
>>> > Talend - http://www.talend.com
>>>
>>
>>
>> --
>> ================
>> Ruoyun Huang
>>
>>
Re: Enforce javadoc comments in public methods?
Posted by Ankur Goenka <go...@google.com>.
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 <ru...@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 <ro...@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é <jb...@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é
>> > jbonofre@apache.org
>> > http://blog.nanthrax.net
>> > Talend - http://www.talend.com
>>
>
>
> --
> ================
> Ruoyun Huang
>
>
Re: Enforce javadoc comments in public methods?
Posted by Ruoyun Huang <ru...@google.com>.
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 <ro...@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é <jb...@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é
> > jbonofre@apache.org
> > http://blog.nanthrax.net
> > Talend - http://www.talend.com
>
--
================
Ruoyun Huang
Re: Enforce javadoc comments in public methods?
Posted by Robert Bradshaw <ro...@google.com>.
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é <jb...@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é
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com
Re: Enforce javadoc comments in public methods?
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
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é
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com