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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > wrote: > > > > > +1 for the voting. > > > > > > > > > Regards > > > Dapeng > > > > > > -----Original Message----- > > > From: Ma, Junjie [mailto:[email protected]] > > > Sent: Friday, October 16, 2015 4:18 PM > > > To: [email protected] > > > 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:[email protected]] > > > Sent: Friday, October 16, 2015 3:18 PM > > > To: [email protected] > > > 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 > > >
