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