You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sravya Tirukkovalur <sr...@cloudera.com> on 2015/11/17 21:40:38 UTC
Re: [VOTE] Adopt Sentry Project Coding Style Guide and Best Practices
Wondering if we have closed the loop here? Lenni, will you be able to add a
wiki page to capture this if not already done?
On Fri, Oct 30, 2015 at 9:42 AM, Lenni Kuff <ls...@cloudera.com> wrote:
> Thanks Sravya. I think your suggestions make sense. There are some
> recommendations for comments already included in here, but I will make it
> more explicit. I also propose we use 90 characters per lines rather than 80
> characters per line, since most new monitors have higher resolution. I
> think we should put these in place (with feedback integrated) and see how
> it goes - we can fine tune later.
>
> Thanks,
> Lenni
>
> On Mon, Oct 19, 2015 at 3:40 PM, Sravya Tirukkovalur <sr...@cloudera.com>
> wrote:
>
> > +1
> >
> > Great list! Some more suggestions:
> >
> > Can we also encourage test driven development?
> > - Make sure all new features have extensive unit tests
> > - We should be able to use mocks where safe to speed up test execution.
> >
> > - Use descriptive names for variables and functions.
> >
> > And should we have a recommendation for comments?
> >
> >
> >
> > On Fri, Oct 16, 2015 at 12:17 PM, Lenni Kuff <ls...@cloudera.com>
> wrote:
> >
> > > Let's keep this thread focused on the coding style. Integration with
> > > FindBugs is useful, but should be separated from this discussion.
> > >
> > > Thanks,
> > > Lenni
> > >
> > > On Fri, Oct 16, 2015 at 12:12 PM, Gregory Chanan <gchanan@cloudera.com
> >
> > > wrote:
> > >
> > > > Can we make sure there is a way to avoid running the automatic tool?
> > > I.e.
> > > > either the tool is a separate command that the precommit/release
> > process
> > > > runs, or the tool runs under normal mvn compile/test but has a flag
> to
> > > > disable?
> > > >
> > > > I've seen a couple of cases where these things run while you are
> > > developing
> > > > and it's pretty invasive.
> > > >
> > > > Greg
> > > >
> > > > On Fri, Oct 16, 2015 at 10:50 AM, Anne Yu <an...@cloudera.com>
> wrote:
> > > >
> > > > > +1 for Lenni's proposed coding style;
> > > > >
> > > > > also +1 for Colin's proposal of integrating an automatic tool into
> > > sentry
> > > > > code base. whenever run maven, the code can be validated through
> such
> > > > tool.
> > > > > Not familiar with FindBugs plugin if it can be integrated into
> maven.
> > > > Maybe
> > > > > can open another voting thread for any suggested style tool?
> > > > >
> > > > > Best,
> > > > > Anne
> > > > >
> > > > > On Fri, Oct 16, 2015 at 1:33 AM, Sun, Dapeng <dapeng.sun@intel.com
> >
> > > > wrote:
> > > > >
> > > > > > +1 for the voting.
> > > > > >
> > > > > >
> > > > > > Regards
> > > > > > Dapeng
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ma, Junjie [mailto:junjie.ma@intel.com]
> > > > > > Sent: Friday, October 16, 2015 4:18 PM
> > > > > > To: dev@sentry.incubator.apache.org
> > > > > > Subject: RE: [VOTE] Adopt Sentry Project Coding Style Guide and
> > Best
> > > > > > Practices
> > > > > >
> > > > > > +1, and do we need to add FindBugs plugin to check the code?
> > > > > >
> > > > > > Best regards,
> > > > > >
> > > > > > Colin Ma(Ma Jun Jie)
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: Lenni Kuff [mailto:lskuff@cloudera.com]
> > > > > > Sent: Friday, October 16, 2015 3:18 PM
> > > > > > To: dev@sentry.incubator.apache.org
> > > > > > Subject: [VOTE] Adopt Sentry Project Coding Style Guide and Best
> > > > > Practices
> > > > > >
> > > > > > Hi,
> > > > > > As a follow up to this [1] discussion thread, I would like to
> > > propose a
> > > > > > vote to adopt a formal project coding style guide and coding best
> > > > > practices
> > > > > > for all new contributions. The style guide proposed is similar to
> > the
> > > > > > Hadoop project's style guide [2], with a few items removed that
> did
> > > not
> > > > > > make sense for Sentry. I have attached the full details at the
> > bottom
> > > > of
> > > > > > this mail. Please respond if you have any concerns or think we
> > should
> > > > > > add/remove/modify anything proposed.
> > > > > >
> > > > > > Vote will be opened for 72 hours.
> > > > > >
> > > > > > [ ] +1 approve
> > > > > > [ ] +0 no opinion
> > > > > > [ ] -1 disapprove (and reason why)
> > > > > >
> > > > > > Thanks,
> > > > > > Lenni
> > > > > >
> > > > > >
> > > > > > [1] -
> > > > > >
> > > >
> > http://mail-archives.apache.org/mod_mbox/sentry-dev/201508.mbox/browser
> > > > > > [2] - https://wiki.apache.org/hadoop/CodeReviewChecklist
> > > > > >
> > > > > > *Coding Style*
> > > > > > This is a modified version of the Hadoop project's code style
> > guide <
> > > > > > https://wiki.apache.org/hadoop/CodeReviewChecklist>.
> > > > > >
> > > > > > *new code:*
> > > > > >
> > > > > > -
> > > > > >
> > > > > > follows Sun's code conventions
> > > > > > <
> > > > > >
> > > > >
> > > >
> > >
> >
> http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html
> > > > > > >
> > > > > > except
> > > > > > indentation is 2 spaces, not 4
> > > > > >
> > > > > > *changes to existing code:*
> > > > > >
> > > > > > - maintains existing style
> > > > > >
> > > > > > Documentation
> > > > > >
> > > > > > *Internal javadoc:*
> > > > > >
> > > > > > - accurate, sufficient for future maintainability
> > > > > >
> > > > > > *Public API javadoc:*
> > > > > >
> > > > > > - accurate, sufficient for developers to code against
> > > > > > - follows standard javadoc conventions
> > > > > > - system properties, configuration options, and resources
> > covered
> > > > > > - illegal arguments are properly documented as appropriate
> > > > > > - package and overview javadoc are updated as appropriate
> > > > > >
> > > > > > Coding Best Practices
> > > > > >
> > > > > > - implementation matches what the documentation says
> > > > > > -
> > > > > >
> > > > > > logger name is effectively the result of Class.getName()
> > > > > > -
> > > > > >
> > > > > > class & member access - as restricted as it can be (subject to
> > > > testing
> > > > > > requirements)
> > > > > > -
> > > > > >
> > > > > > appropriate NullPointerException and IllegalArgumentException
> > > > argument
> > > > > > checks
> > > > > > -
> > > > > >
> > > > > > look for accidental propagation of exceptions
> > > > > > -
> > > > > >
> > > > > > look for unanticipated runtime exceptions
> > > > > > -
> > > > > > - try-finally used as necessary to restore consistent state
> > > > > > - possible deadlocks - look for inconsistent locking order
> > > > > > - race conditions - look for missing or inadequate
> > synchronization
> > > > > > - consistent synchronization - always locking the same
> object(s)
> > > > > > - look for synchronization or documentation saying there's no
> > > > > > synchronization
> > > > > > - look for possible performance problems
> > > > > > - look at boundary conditions for problems
> > > > > > - configuration entries are retrieved/set via setter/getter
> > > methods
> > > > > > - implementation details do NOT leak into interfaces
> > > > > > - variables and arguments should be interfaces where possible
> > > > > > -
> > > > > >
> > > > > > if equals is overridden then hashCode is overridden (and vice
> > > versa)
> > > > > > -
> > > > > >
> > > > > > objects are checked (instanceof) for appropriate type before
> > > casting
> > > > > > (use generics if possible)
> > > > > > - public API changes have been publicly discussed
> > > > > > - use of static member variables should be used with caution
> > > > > >
> > > > > > Tests
> > > > > >
> > > > > > -
> > > > > >
> > > > > > unit tests exist for bug fixes and new features, or a
> rationale
> > is
> > > > > given
> > > > > > in Jira for why there is no test
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Anne
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Sravya Tirukkovalur
> >
>
--
Sravya Tirukkovalur
Re: [VOTE] Adopt Sentry Project Coding Style Guide and Best Practices
Posted by Lenni Kuff <ls...@cloudera.com>.
Sorry for the delay here. Yes, I think we are all in agreement. I had some
issues with website access so wasn't able to make the updates yet. I'll be
back from vacation next week and will try to get the permission issues
resolved and get this updated.
Thanks.
Lenni
On Tue, Nov 17, 2015 at 12:40 PM, Sravya Tirukkovalur <sr...@cloudera.com>
wrote:
> Wondering if we have closed the loop here? Lenni, will you be able to add a
> wiki page to capture this if not already done?
>
> On Fri, Oct 30, 2015 at 9:42 AM, Lenni Kuff <ls...@cloudera.com> wrote:
>
> > Thanks Sravya. I think your suggestions make sense. There are some
> > recommendations for comments already included in here, but I will make it
> > more explicit. I also propose we use 90 characters per lines rather than
> 80
> > characters per line, since most new monitors have higher resolution. I
> > think we should put these in place (with feedback integrated) and see how
> > it goes - we can fine tune later.
> >
> > Thanks,
> > Lenni
> >
> > On Mon, Oct 19, 2015 at 3:40 PM, Sravya Tirukkovalur <
> sravya@cloudera.com>
> > wrote:
> >
> > > +1
> > >
> > > Great list! Some more suggestions:
> > >
> > > Can we also encourage test driven development?
> > > - Make sure all new features have extensive unit tests
> > > - We should be able to use mocks where safe to speed up test execution.
> > >
> > > - Use descriptive names for variables and functions.
> > >
> > > And should we have a recommendation for comments?
> > >
> > >
> > >
> > > On Fri, Oct 16, 2015 at 12:17 PM, Lenni Kuff <ls...@cloudera.com>
> > wrote:
> > >
> > > > Let's keep this thread focused on the coding style. Integration with
> > > > FindBugs is useful, but should be separated from this discussion.
> > > >
> > > > Thanks,
> > > > Lenni
> > > >
> > > > On Fri, Oct 16, 2015 at 12:12 PM, Gregory Chanan <
> gchanan@cloudera.com
> > >
> > > > wrote:
> > > >
> > > > > Can we make sure there is a way to avoid running the automatic
> tool?
> > > > I.e.
> > > > > either the tool is a separate command that the precommit/release
> > > process
> > > > > runs, or the tool runs under normal mvn compile/test but has a flag
> > to
> > > > > disable?
> > > > >
> > > > > I've seen a couple of cases where these things run while you are
> > > > developing
> > > > > and it's pretty invasive.
> > > > >
> > > > > Greg
> > > > >
> > > > > On Fri, Oct 16, 2015 at 10:50 AM, Anne Yu <an...@cloudera.com>
> > wrote:
> > > > >
> > > > > > +1 for Lenni's proposed coding style;
> > > > > >
> > > > > > also +1 for Colin's proposal of integrating an automatic tool
> into
> > > > sentry
> > > > > > code base. whenever run maven, the code can be validated through
> > such
> > > > > tool.
> > > > > > Not familiar with FindBugs plugin if it can be integrated into
> > maven.
> > > > > Maybe
> > > > > > can open another voting thread for any suggested style tool?
> > > > > >
> > > > > > Best,
> > > > > > Anne
> > > > > >
> > > > > > On Fri, Oct 16, 2015 at 1:33 AM, Sun, Dapeng <
> dapeng.sun@intel.com
> > >
> > > > > wrote:
> > > > > >
> > > > > > > +1 for the voting.
> > > > > > >
> > > > > > >
> > > > > > > Regards
> > > > > > > Dapeng
> > > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ma, Junjie [mailto:junjie.ma@intel.com]
> > > > > > > Sent: Friday, October 16, 2015 4:18 PM
> > > > > > > To: dev@sentry.incubator.apache.org
> > > > > > > Subject: RE: [VOTE] Adopt Sentry Project Coding Style Guide and
> > > Best
> > > > > > > Practices
> > > > > > >
> > > > > > > +1, and do we need to add FindBugs plugin to check the code?
> > > > > > >
> > > > > > > Best regards,
> > > > > > >
> > > > > > > Colin Ma(Ma Jun Jie)
> > > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Lenni Kuff [mailto:lskuff@cloudera.com]
> > > > > > > Sent: Friday, October 16, 2015 3:18 PM
> > > > > > > To: dev@sentry.incubator.apache.org
> > > > > > > Subject: [VOTE] Adopt Sentry Project Coding Style Guide and
> Best
> > > > > > Practices
> > > > > > >
> > > > > > > Hi,
> > > > > > > As a follow up to this [1] discussion thread, I would like to
> > > > propose a
> > > > > > > vote to adopt a formal project coding style guide and coding
> best
> > > > > > practices
> > > > > > > for all new contributions. The style guide proposed is similar
> to
> > > the
> > > > > > > Hadoop project's style guide [2], with a few items removed that
> > did
> > > > not
> > > > > > > make sense for Sentry. I have attached the full details at the
> > > bottom
> > > > > of
> > > > > > > this mail. Please respond if you have any concerns or think we
> > > should
> > > > > > > add/remove/modify anything proposed.
> > > > > > >
> > > > > > > Vote will be opened for 72 hours.
> > > > > > >
> > > > > > > [ ] +1 approve
> > > > > > > [ ] +0 no opinion
> > > > > > > [ ] -1 disapprove (and reason why)
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Lenni
> > > > > > >
> > > > > > >
> > > > > > > [1] -
> > > > > > >
> > > > >
> > >
> http://mail-archives.apache.org/mod_mbox/sentry-dev/201508.mbox/browser
> > > > > > > [2] - https://wiki.apache.org/hadoop/CodeReviewChecklist
> > > > > > >
> > > > > > > *Coding Style*
> > > > > > > This is a modified version of the Hadoop project's code style
> > > guide <
> > > > > > > https://wiki.apache.org/hadoop/CodeReviewChecklist>.
> > > > > > >
> > > > > > > *new code:*
> > > > > > >
> > > > > > > -
> > > > > > >
> > > > > > > follows Sun's code conventions
> > > > > > > <
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html
> > > > > > > >
> > > > > > > except
> > > > > > > indentation is 2 spaces, not 4
> > > > > > >
> > > > > > > *changes to existing code:*
> > > > > > >
> > > > > > > - maintains existing style
> > > > > > >
> > > > > > > Documentation
> > > > > > >
> > > > > > > *Internal javadoc:*
> > > > > > >
> > > > > > > - accurate, sufficient for future maintainability
> > > > > > >
> > > > > > > *Public API javadoc:*
> > > > > > >
> > > > > > > - accurate, sufficient for developers to code against
> > > > > > > - follows standard javadoc conventions
> > > > > > > - system properties, configuration options, and resources
> > > covered
> > > > > > > - illegal arguments are properly documented as appropriate
> > > > > > > - package and overview javadoc are updated as appropriate
> > > > > > >
> > > > > > > Coding Best Practices
> > > > > > >
> > > > > > > - implementation matches what the documentation says
> > > > > > > -
> > > > > > >
> > > > > > > logger name is effectively the result of Class.getName()
> > > > > > > -
> > > > > > >
> > > > > > > class & member access - as restricted as it can be (subject
> to
> > > > > testing
> > > > > > > requirements)
> > > > > > > -
> > > > > > >
> > > > > > > appropriate NullPointerException and
> IllegalArgumentException
> > > > > argument
> > > > > > > checks
> > > > > > > -
> > > > > > >
> > > > > > > look for accidental propagation of exceptions
> > > > > > > -
> > > > > > >
> > > > > > > look for unanticipated runtime exceptions
> > > > > > > -
> > > > > > > - try-finally used as necessary to restore consistent state
> > > > > > > - possible deadlocks - look for inconsistent locking order
> > > > > > > - race conditions - look for missing or inadequate
> > > synchronization
> > > > > > > - consistent synchronization - always locking the same
> > object(s)
> > > > > > > - look for synchronization or documentation saying there's
> no
> > > > > > > synchronization
> > > > > > > - look for possible performance problems
> > > > > > > - look at boundary conditions for problems
> > > > > > > - configuration entries are retrieved/set via setter/getter
> > > > methods
> > > > > > > - implementation details do NOT leak into interfaces
> > > > > > > - variables and arguments should be interfaces where
> possible
> > > > > > > -
> > > > > > >
> > > > > > > if equals is overridden then hashCode is overridden (and
> vice
> > > > versa)
> > > > > > > -
> > > > > > >
> > > > > > > objects are checked (instanceof) for appropriate type before
> > > > casting
> > > > > > > (use generics if possible)
> > > > > > > - public API changes have been publicly discussed
> > > > > > > - use of static member variables should be used with caution
> > > > > > >
> > > > > > > Tests
> > > > > > >
> > > > > > > -
> > > > > > >
> > > > > > > unit tests exist for bug fixes and new features, or a
> > rationale
> > > is
> > > > > > given
> > > > > > > in Jira for why there is no test
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > > Anne
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Sravya Tirukkovalur
> > >
> >
>
>
>
> --
> Sravya Tirukkovalur
>