Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Received-SPF: pass (nike.apache.org: domain of david.medinets@gmail.com
 designates 209.85.212.171 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAL5zq9ZR4ScJNFhTXfiDRyFUOfshy_A-tcvW709Hdm8-cyGbZg@mail.gmail.com>
References: 
 <CAL5zq9bkbbYi5CfzXsqFJDSWV5ekLGN3iL4Gvibd59jmVpmSDQ@mail.gmail.com>
	<CAL5zq9b2NtxwO9Ro3dGT-chSU0wwu5xgordpeXc+it+_KMBkcQ@mail.gmail.com>
	<CAL5zq9bPYfooK9safiEiEAFHK66jisZvedDCAEG=UEa7iNvRxA@mail.gmail.com>
	<549B212E.5060901@gmail.com>
	<CAL5zq9ZR4ScJNFhTXfiDRyFUOfshy_A-tcvW709Hdm8-cyGbZg@mail.gmail.com>
Date: Thu, 25 Dec 2014 12:06:17 -0500
Message-ID: 
 <CAOiJXP6co13gP9tky2EBd+2zamHqMs7Wy_kqWvzxhdJHWPjS_Q@mail.gmail.com>
Subject: Re: Checkstyle Notes
From: David Medinets <david.medinets@gmail.com>
To: accumulo-dev <dev@accumulo.apache.org>
Content-Type: text/plain; charset=UTF-8

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
>>>>>
>>>>>
>>>>
>>>