+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 <danolive...@google.com> > wrote: > >> +1 >> >> I like this idea, especially with the line number requirement. The exact >> number of lines is debatable, but you could go as low as 10 lines and that >> would exclude any trivial setters and getters. Even better might be if it's >> possible to configure checkstyle to ignore this for getters and setters (I >> don't know if checkstyle supports this, but I know that other tools are >> able to auto-detect getters and setters). >> >> I'm not dead-set against having annotation to suppress the comment, but >> it carries the risk that code will be left un-commented because both the >> dev and reviewer think it's self-explanatory, and then someone new to the >> codebase finds it confusing. >> >> On Mon, Jan 7, 2019 at 11:31 AM Ankur Goenka <goe...@google.com> wrote: >> >>> I think it makes sense. >>> Having an annotation to suppress this check for a method/class instead >>> of adding trivial comment would be useful. >>> >>> On Mon, Jan 7, 2019 at 9:53 AM Ruoyun Huang <ruo...@google.com> wrote: >>> >>>> Yeah. Agree there is no reason to enforce anything for trivial methods >>>> like setter/getter. >>>> >>>> What I meant is to enforce only for a method that is *BOTH* 1) public >>>> method 2) has longer than N lines. >>>> >>>> sorry for not making the proposal clear enough in the original message, >>>> it should've better titled "enforce ... on non-trivial public methods". >>>> >>>> >>>> >>>> On Mon, Jan 7, 2019 at 1:31 AM Robert Bradshaw <rober...@google.com> >>>> wrote: >>>> >>>>> IMHO, requiring comments on trivial methods like setters and getters >>>>> is often a net negative, but setting some standard could be useful. >>>>> >>>>> On Mon, Jan 7, 2019 at 7:35 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>>>> wrote: >>>>> > >>>>> > Hi, >>>>> > >>>>> > for the presence of a comment on public method, it's a good idea. >>>>> Now, >>>>> > about the number of lines, not sure it's a good idea. I'm thinking >>>>> about >>>>> > the getter/setter which are public. Most of the time, the comment is >>>>> > pretty simple (and useless ;)). >>>>> > >>>>> > Regards >>>>> > JB >>>>> > >>>>> > On 07/01/2019 04:35, Ruoyun Huang wrote: >>>>> > > Hi, everyone, >>>>> > > >>>>> > > >>>>> > > We were wondering whether it is a good idea to make checkstyle >>>>> > > enforce public method comments. Our current behavior of JavaDoc >>>>> check is: >>>>> > > >>>>> > > 1. >>>>> > > >>>>> > > Missing Class javadoc comment is reported as error. >>>>> > > >>>>> > > 2. >>>>> > > >>>>> > > Method comment missing is explicitly allowed. see [1]. It is >>>>> not >>>>> > > even shown as warning. >>>>> > > >>>>> > > 3. >>>>> > > >>>>> > > The actual javadoc target gives warning when certain tags are >>>>> > > missing in javadoc, but not if the whole comment is missing. >>>>> > > >>>>> > > >>>>> > > How about we enforce method comments for **1) public method and >>>>> 2) >>>>> > > method that is longer than N lines**. (N=~30 seems a good number, >>>>> > > leading to ~50 violations in current repository). I can find out >>>>> the >>>>> > > corresponding contributors to fill in the missing comments, before >>>>> we >>>>> > > turning the check fully on. >>>>> > > >>>>> > > >>>>> > > One caveat though is that we might want skip this check on test >>>>> code, >>>>> > > but I am not sure yet if our current setup can easily handle >>>>> separated >>>>> > > rules for main code versus test code. >>>>> > > >>>>> > > >>>>> > > Is this a good idea? Thoughts and suggestions? >>>>> > > >>>>> > > >>>>> > > [1] >>>>> > > >>>>> https://github.com/apache/beam/blame/5ceffb246c0c38ad68dd208e951a1f39c90ef85c/sdks/java/build-tools/src/main/resources/beam/checkstyle.xml#L111 >>>>> > > >>>>> > > >>>>> > > Cheers, >>>>> > > >>>>> > >>>>> > -- >>>>> > Jean-Baptiste Onofré >>>>> > jbono...@apache.org >>>>> > http://blog.nanthrax.net >>>>> > Talend - http://www.talend.com >>>>> >>>> >>>> >>>> -- >>>> ================ >>>> Ruoyun Huang >>>> >>>>
