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