accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <ctubb...@apache.org>
Subject Re: Checkstyle Notes
Date Fri, 26 Dec 2014 03:18:44 GMT
Because it's still a supported branch, and some of the javadoc problems
affect it, too. I also figured that if we do the reformatting thing, I
might as well start with 1.5, and deal with the merge conflicts myself, so
others don't have to when they apply bugfixes to older supported branches.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Thu, Dec 25, 2014 at 12:06 PM, David Medinets <david.medinets@gmail.com>
wrote:

> Why v1.5 instead of the latest?
>
> On Wed, Dec 24, 2014 at 3:34 PM, Christopher <ctubbsii@apache.org> wrote:
> > I pushed some changes to the 1.5 branch to GitHub:
> > https://github.com/ctubbsii/accumulo/tree/ACCUMULO-3451
> > There's 3 commits:
> > 1. reformat everything (also gets rid of tab characters, organizes
> imports,
> > and strips whitespace off the ends of lines)
> > 2. Add checkstyle enforcer rules to pom
> > 3. Make changes to comply with checkstyle rules (some of the compliance
> is
> > satisfied by step 1, which made this easier and more minimal)
> >
> > I haven't merged to other branches, or pushed anything to ASF git.
> >
> >
> > --
> > Christopher L Tubbs II
> > http://gravatar.com/ctubbsii
> >
> > On Wed, Dec 24, 2014 at 3:25 PM, Josh Elser <josh.elser@gmail.com>
> wrote:
> >
> >> At this point, I'd be in favor of just "fixing" the codebase. I spend so
> >> much time undo'ing formatting changes automatically made by Eclipse that
> >> are unrelated to my actual changes in an attempt to keep things
> minimized.
> >>
> >> Merging will be more painful (hopefully Git is smart enough for most of
> >> it?), but maybe it's for the best? I'm not entirely sure.
> >>
> >> Agreed in that we should avoid doing it twice if we're going to do it at
> >> all. Do you have some concrete changes in mind that we could poke around
> >> with?
> >>
> >> Christopher wrote:
> >>
> >>> Working on ACCUMULO-3451, what I found was that there's virtually no
> >>> subset
> >>> of rules I can apply that will not force some significant amount of our
> >>> code to change in order to conform to those rules. it would be *much*
> >>> easier if I just formatted the whole codebase first. I wouldn't want
> to do
> >>> that twice in a short amount of time, though, if we're going to adopt
> >>> google-styleguide any time soon.
> >>>
> >>> It seems that no matter what, enforcement of standards to prevent
> further
> >>> quality decay, when we haven't been complying very well up to this
> point,
> >>> is going to result in some disruption. The trick is getting the right
> >>> balance/utility. Any thoughts?
> >>>
> >>>
> >>> --
> >>> Christopher L Tubbs II
> >>> http://gravatar.com/ctubbsii
> >>>
> >>> On Tue, Dec 23, 2014 at 11:34 PM, Christopher<ctubbsii@apache.org>
> >>> wrote:
> >>>
> >>>  One problem I ran into, by the way, is that Eclipse does not actually
> >>>> enforce the max line length. There's tons of bugs which have been open
> >>>> for
> >>>> years, which prevent the formatter from wrapping in special
> >>>> circumstances.
> >>>> The most common was simply that it ignored trailing punctuation. In
> other
> >>>> words, if the only thing that would have wrapped was punctuation, it
> >>>> doesn't wrap. This appears to be a competition between the rule about
> >>>> putting parens/semicolons/braces on the same line and the rule for
> >>>> wrapping. The same line appears to win.
> >>>>
> >>>> Checkstyle, on the other hand, gives a hard error on exceeding the
> line
> >>>> length (which it should). This means that a file formatted with a
> >>>> formatter
> >>>> set with a max line length of X may produce lines longer than X, and
> will
> >>>> cause checkstyle to fail. The workaround is to manually insert
> >>>> @formatter:off and @formatter:on lines so you can manually tweak to
> make
> >>>> checkstyle happy without worrying about Eclipse reformatting.
> Currently,
> >>>> these tags are not available in the google-styleguide. I have an open
> >>>> issue
> >>>> about that, with a patch:
> >>>> https://code.google.com/p/google-styleguide/issues/detail?id=27 .
> >>>> There's
> >>>> also a few random bugs in the checkstyle implementation of the
> >>>> google-styleguide (
> https://github.com/checkstyle/checkstyle/issues/533
> >>>> and others), but these aren't a big deal, and in any case, only
> affect us
> >>>> if we decide to adopt google-styleguide's rulesets.
> >>>>
> >>>>
> >>>> --
> >>>> Christopher L Tubbs II
> >>>> http://gravatar.com/ctubbsii
> >>>>
> >>>> On Tue, Dec 23, 2014 at 1:24 PM, Christopher<ctubbsii@apache.org>
> >>>> wrote:
> >>>>
> >>>>  Devs,
> >>>>>
> >>>>> So, I spent some time yesterday investigating the application of
a
> >>>>> CheckStyle configuration to apply to our builds.
> >>>>>
> >>>>> I started with the CheckStyle implementation[1] of the Google Style
> >>>>> Guide[2], and a fully formatted Accumulo code base (according to
our
> >>>>> current Eclipse formatter standards[3]). I then configured the
> >>>>> maven-checkstyle-plugin[4][5] to run during a build and built
> >>>>> repeatedly,
> >>>>> whittled down some of the strictness of the Google style and fixed
> some
> >>>>> of
> >>>>> our errors, to get at a minimal checkstyle configuration that might
> be
> >>>>> able
> >>>>> to add some additional checks during our build. I found at least
one
> >>>>> bug[6]
> >>>>> doing this and evaluating the resulting warnings, and documented
> some of
> >>>>> the common problems.
> >>>>>
> >>>>> Now, I'm not necessarily arguing for switching to adopting the Google
> >>>>> Style Guide for our standards today (we can consider voting on that
> in a
> >>>>> new thread). However, I think it might be beneficial to adopt the
> Google
> >>>>> Style Guide, especially because that project has formatters for
> several
> >>>>> standard IDEs, and because the latest version of CheckStyle has
a
> >>>>> ruleset
> >>>>> which conforms to it, which enables the Maven tooling. So, if
> anybody is
> >>>>> interesting in us doing that, I would certainly get behind it (and
> could
> >>>>> help make it happen: reformatting, merging, and applying build
> tooling).
> >>>>> One thing I like about the Google Style Guide, in particular, is
> that it
> >>>>> doesn't just provide tools to enforce it, it also documents the
> standard
> >>>>> with words and reasoning, so we have something to consult, even
if
> >>>>> you're
> >>>>> not using an IDE or any of the tools.
> >>>>>
> >>>>> As a result of this, I am going to add something to start enforcing
> >>>>> checks for some of the javadoc problems. We can add more checks
> later,
> >>>>> or
> >>>>> we can adopt the Google Style.
> >>>>>
> >>>>> In the meantime, here's a list of the most common style problems
I
> >>>>> observed (note: these aren't necessarily "bad", just non-conforming):
> >>>>>
> >>>>> Line length violations (we're currently using 160, but Google Style
> >>>>> Guide
> >>>>> allows 80 or 100, with 100 enforced in the tools by default; many
of
> our
> >>>>> lines exceed the 160).
> >>>>> Escaped unicode in string literals (especially when they represent
a
> >>>>> printable character which should just be inserted directly, instead
> of
> >>>>> encoded).
> >>>>> Use of lower-case L for long literals (prefer 10L over 10l).
> >>>>> Bad import order, use of wildcards.
> >>>>> Missing empty line between license header and package declaration.
> >>>>> Extra whitespace around generic parameters.
> >>>>> Bad package/class/method/parameter/local variable naming conventions
> >>>>> (like single character variables outside of loops, or use of
> >>>>> underscore/non-standard camelCase).
> >>>>> Missing optional braces for blocks (see a representatively bad
> example
> >>>>> at
> >>>>> CompactionInfo:lines 74-82).
> >>>>> Keywords out of order from JLS recommendations (e.g. static public
> vs.
> >>>>> public static).
> >>>>> Overloaded methods aren't grouped together.
> >>>>> Multiple statements per line, multiple variable declarations per
> line.
> >>>>> Array brackets should be on type (String[] names;), not variable
name
> >>>>> (String names[];).
> >>>>> Switch statements sometimes omit default case or fall through.
> >>>>> Distance between local variable declaration and the first time it
is
> >>>>> used
> >>>>> is sometimes too long (many lines).
> >>>>> All uppercase abbreviations in names aren't avoided (LockID should
be
> >>>>> LockId).
> >>>>> Operators should start the next line, when wrapping (like
> concatenation
> >>>>> of two long string literals).
> >>>>> Empty catch blocks (catch blocks should throw an AssertionError
if
> it's
> >>>>> supposed to be impossible to reach, or a comment about why it's
safe
> to
> >>>>> ignore).
> >>>>> Some files have more than one top-level class. They should be in
> their
> >>>>> own file.
> >>>>> Braces not on same line as statement (like "} else" with "{" on
next
> >>>>> line).
> >>>>>
> >>>>> Missing javadocs on public methods.
> >>>>> Missing paragraph markers(<p>) in javadoc paragraphs.
> >>>>> Missing javadoc sentence descriptions.
> >>>>> Javadoc tags out of order from standards.
> >>>>> Missing html close tags in javadocs (<b>  but no</b>,<ul>
 but
> no</ul>)
> >>>>> Use of<  and>  in javadoc instead of&lt; and&gt; or
{@code ...}
> >>>>>
> >>>>> [1]:
> >>>>>
> https://github.com/checkstyle/checkstyle/blob/master/google_checks.xml
> >>>>> [2]: https://code.google.com/p/google-styleguide/
> >>>>> [3]:
> >>>>> https://github.com/apache/accumulo/blob/master/contrib/
> >>>>> Eclipse-Accumulo-Codestyle.xml
> >>>>> [4]:
> >>>>>
> http://maven.apache.org/plugins/maven-checkstyle-plugin/check-mojo.html
> >>>>> [5]: http://stackoverflow.com/a/27359107/196405
> >>>>> [6]: https://issues.apache.org/jira/browse/ACCUMULO-3448
> >>>>>
> >>>>> --
> >>>>> Christopher L Tubbs II
> >>>>> http://gravatar.com/ctubbsii
> >>>>>
> >>>>>
> >>>>
> >>>
>

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