+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 >> >>
