hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Dimiduk <ndimi...@gmail.com>
Subject Re: Better code quality with more checkstyles
Date Tue, 17 Nov 2015 21:20:17 GMT
The problem with a fixup "rampage" is it will very like introduce conflicts
with any patches that are in review process. We have no issue with general
code cleanup. The problem is introducing more work for
contributors/reviewers while it's happening. The cleanup needs to happen in
tandem with compile-time enforcement of the rules so that we don't let the
bit-rott set back in again.

So really, it's about coordinating the roll-out with minimal impact on
active development. Proposals?

On Tue, Nov 17, 2015 at 11:55 AM, Apekshit Sharma <appy@cloudera.com> wrote:

> Emm...do no replies mean "don't care" which means silent yes? :-)
>
> On Tue, Nov 10, 2015 at 3:11 PM, Apekshit Sharma <appy@cloudera.com>
> wrote:
>
> > Last sunday, when i was bored, i thought  'let's fix some naive stuff,
> > oh.. checkstyles!'. After some time and few changes, I realized what can
> be
> > more boring than idling around...repeating large mechanical change
> across 5
> > branches :-(. And can't just do it for single branch.
> >
> > Then, looking at history of checkstyle errors:
> > 5th sept: 3815
> > 7th sept: 1834  <-- what happened in last 2 days?
> > 5th nov: 1726
> > Seems like they do get fixed over time.
> >
> > So the idea here is, increase the code quality bar to see how good/bad we
> > are, basically know where we stand, and work incrementally towards
> getting
> > better.
> > Personally, in all my patches, i'll fix some checkstyle issues only
> around
> > the code am touching. But if someone wants to go on a rampage and fix
> lots
> > of them at once, wohooo!
> >
> >
> >
> >
> >
> >
> > On Tue, Nov 10, 2015 at 8:51 AM, Jonathan Hsieh <jon@cloudera.com>
> wrote:
> >
> >> Is the goal just to incrementally fix these as we touch code or do you
> >> plan
> >> some mass check style cleanup?
> >>
> >> I've recently started contributing again and like how the checkstyle
> >> scripting enforces a monotonicly decreasing # of violations.
> >>
> >> On Monday, November 9, 2015, Apekshit Sharma <appy@cloudera.com> wrote:
> >>
> >> > Sometimes, looking at certain things in our code makes me think, 'How
> >> did
> >> > it  get in, there should be some kind of check for it!'.
> >> > Looked at different checkstyles rules and I think we can enable the
> >> > following ones to improve the formatting quality of our codebase.
> >> >
> >> > ImportOrder
> >> > <http://checkstyle.sourceforge.net/config_imports.html#ImportOrder>
:
> >> keep
> >> > imports in sorted order
> >> >
> >> > LeftCurly <
> >> http://checkstyle.sourceforge.net/config_blocks.html#LeftCurly>
> >> > :
> >> > Placement of left curly brace. Does 'eol' sounds right setting?
> >> >
> >> > NeedBraces <
> >> > http://checkstyle.sourceforge.net/config_blocks.html#NeedBraces> :
> >> > braces around code blocks
> >> >
> >> > JavadocTagContinuationIndentation
> >> > <
> >> >
> >>
> http://checkstyle.sourceforge.net/config_javadoc.html#JavadocTagContinuationIndentation
> >> > >
> >> > :
> >> > Avoid weird indentations in javadocs
> >> >
> >> > NonEmptyAtclauseDescription
> >> > <
> >> >
> >>
> http://checkstyle.sourceforge.net/config_javadoc.html#NonEmptyAtclauseDescription
> >> > >
> >> > :
> >> > We have sooooo many empty javadoc @ clauses. This'll take care of it.
> >> >
> >> > Indentation <
> >> > http://checkstyle.sourceforge.net/config_misc.html#Indentation> :
> >> > Bad indentation hurts code readability. We have indentation
> guidelines,
> >> > should be fine enforcing them.
> >> >
> >> > - Apekshit
> >> >
> >>
> >>
> >> --
> >> // Jonathan Hsieh (shay)
> >> // HBase Tech Lead, Software Engineer, Cloudera
> >> // jon@cloudera.com // @jmhsieh
> >>
> >
> >
> >
> > --
> >
> > Regards
> >
> > Apekshit Sharma | Software Engineer, Cloudera | Palo Alto, California |
> > 650-963-6311
> >
>
>
>
> --
>
> Regards
>
> Apekshit Sharma | Software Engineer, Cloudera | Palo Alto, California |
> 650-963-6311
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message