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 <[email protected]> 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 <[email protected]> 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 <[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 > > > > > > > > > > > > > -- > Sravya Tirukkovalur >
