You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Christopher <ct...@apache.org> on 2014/12/23 19:24:08 UTC
Checkstyle Notes
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 < and > 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
Re: Checkstyle Notes
Posted by David Medinets <da...@gmail.com>.
https://issues.apache.org/jira/browse/ACCUMULO-1158 is the ticket
covering the previous work. I remember somehow I had just turned one
check on and wrote a script file to remove line endings. I understood
totally why the change was rejected. Unfortunately I don't have any
copies of that version.
On Tue, Dec 23, 2014 at 11:21 PM, Christopher <ct...@apache.org> wrote:
> David, I know you made a previous pass at this. I looked for your
> checkstyle rules from back then, but it was in svn, so I gave up. You
> wouldn't happen to have a copy of those rulesets you previously applied, in
> a form I could easily cross-reference, would you?
>
>
> --
> Christopher L Tubbs II
> http://gravatar.com/ctubbsii
>
> On Tue, Dec 23, 2014 at 10:18 PM, David Medinets <da...@gmail.com>
> wrote:
>
>> +1. I'm glad the project has matured enough to consider moving in this
>> direction.
>>
>> On Tue, Dec 23, 2014 at 1:40 PM, John Vines <vi...@apache.org> wrote:
>> > I share sentiments with Josh. I'm all for a more universally supported
>> > standard. Going back down to 100 characters feels limiting and I'm not a
>> > fan, but I'm not going to let that stop me.
>> >
>> > On Tue, Dec 23, 2014 at 1:36 PM, Josh Elser <jo...@gmail.com>
>> wrote:
>> >>
>> >> Thanks for taking the time to do this. I know it can be rather
>> monotonous.
>> >>
>> >> I've read over the Google Java style guide before and liked (just about)
>> >> everything I read. The line limit of 80-100 chars *feels* limiting to me
>> >> (but I've also been told by others my senior that you very much get
>> used to
>> >> that limitation and grow to like it).
>> >>
>> >> Our existing style recommendation is just that (not a requirement), so
>> >> tooling in Maven would be important to me for however we might move.
>> >> Something that will tell me when I did something dumb or knuckle-headed
>> >> will help to remind me.
>> >>
>> >> Christopher 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< and> 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
>> >>>
>> >>>
>>
Re: Checkstyle Notes
Posted by Christopher <ct...@apache.org>.
David, I know you made a previous pass at this. I looked for your
checkstyle rules from back then, but it was in svn, so I gave up. You
wouldn't happen to have a copy of those rulesets you previously applied, in
a form I could easily cross-reference, would you?
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii
On Tue, Dec 23, 2014 at 10:18 PM, David Medinets <da...@gmail.com>
wrote:
> +1. I'm glad the project has matured enough to consider moving in this
> direction.
>
> On Tue, Dec 23, 2014 at 1:40 PM, John Vines <vi...@apache.org> wrote:
> > I share sentiments with Josh. I'm all for a more universally supported
> > standard. Going back down to 100 characters feels limiting and I'm not a
> > fan, but I'm not going to let that stop me.
> >
> > On Tue, Dec 23, 2014 at 1:36 PM, Josh Elser <jo...@gmail.com>
> wrote:
> >>
> >> Thanks for taking the time to do this. I know it can be rather
> monotonous.
> >>
> >> I've read over the Google Java style guide before and liked (just about)
> >> everything I read. The line limit of 80-100 chars *feels* limiting to me
> >> (but I've also been told by others my senior that you very much get
> used to
> >> that limitation and grow to like it).
> >>
> >> Our existing style recommendation is just that (not a requirement), so
> >> tooling in Maven would be important to me for however we might move.
> >> Something that will tell me when I did something dumb or knuckle-headed
> >> will help to remind me.
> >>
> >> Christopher 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< and> 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
> >>>
> >>>
>
Re: Checkstyle Notes
Posted by David Medinets <da...@gmail.com>.
+1. I'm glad the project has matured enough to consider moving in this
direction.
On Tue, Dec 23, 2014 at 1:40 PM, John Vines <vi...@apache.org> wrote:
> I share sentiments with Josh. I'm all for a more universally supported
> standard. Going back down to 100 characters feels limiting and I'm not a
> fan, but I'm not going to let that stop me.
>
> On Tue, Dec 23, 2014 at 1:36 PM, Josh Elser <jo...@gmail.com> wrote:
>>
>> Thanks for taking the time to do this. I know it can be rather monotonous.
>>
>> I've read over the Google Java style guide before and liked (just about)
>> everything I read. The line limit of 80-100 chars *feels* limiting to me
>> (but I've also been told by others my senior that you very much get used to
>> that limitation and grow to like it).
>>
>> Our existing style recommendation is just that (not a requirement), so
>> tooling in Maven would be important to me for however we might move.
>> Something that will tell me when I did something dumb or knuckle-headed
>> will help to remind me.
>>
>> Christopher 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< and> 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
>>>
>>>
Re: Checkstyle Notes
Posted by John Vines <vi...@apache.org>.
I share sentiments with Josh. I'm all for a more universally supported
standard. Going back down to 100 characters feels limiting and I'm not a
fan, but I'm not going to let that stop me.
On Tue, Dec 23, 2014 at 1:36 PM, Josh Elser <jo...@gmail.com> wrote:
>
> Thanks for taking the time to do this. I know it can be rather monotonous.
>
> I've read over the Google Java style guide before and liked (just about)
> everything I read. The line limit of 80-100 chars *feels* limiting to me
> (but I've also been told by others my senior that you very much get used to
> that limitation and grow to like it).
>
> Our existing style recommendation is just that (not a requirement), so
> tooling in Maven would be important to me for however we might move.
> Something that will tell me when I did something dumb or knuckle-headed
> will help to remind me.
>
> Christopher 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< and> 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
>>
>>
Re: Checkstyle Notes
Posted by Josh Elser <jo...@gmail.com>.
Thanks for taking the time to do this. I know it can be rather monotonous.
I've read over the Google Java style guide before and liked (just about)
everything I read. The line limit of 80-100 chars *feels* limiting to me
(but I've also been told by others my senior that you very much get used
to that limitation and grow to like it).
Our existing style recommendation is just that (not a requirement), so
tooling in Maven would be important to me for however we might move.
Something that will tell me when I did something dumb or knuckle-headed
will help to remind me.
Christopher 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< and> 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
>
Re: Checkstyle Notes
Posted by Christopher <ct...@apache.org>.
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 <da...@gmail.com>
wrote:
> Why v1.5 instead of the latest?
>
> On Wed, Dec 24, 2014 at 3:34 PM, Christopher <ct...@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 <jo...@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<ct...@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<ct...@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< and> 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
> >>>>>
> >>>>>
> >>>>
> >>>
>
Re: Checkstyle Notes
Posted by David Medinets <da...@gmail.com>.
Why v1.5 instead of the latest?
On Wed, Dec 24, 2014 at 3:34 PM, Christopher <ct...@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 <jo...@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<ct...@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<ct...@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< and> 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
>>>>>
>>>>>
>>>>
>>>
Re: Checkstyle Notes
Posted by Christopher <ct...@apache.org>.
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 <jo...@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<ct...@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<ct...@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< and> 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
>>>>
>>>>
>>>
>>
Re: Checkstyle Notes
Posted by Josh Elser <jo...@gmail.com>.
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<ct...@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<ct...@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< and> 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
>>>
>>
>
Re: Checkstyle Notes
Posted by Christopher <ct...@apache.org>.
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 <ct...@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 <ct...@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 < and > 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
>>
>
>
Re: Checkstyle Notes
Posted by Christopher <ct...@apache.org>.
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 <ct...@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 < and > 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
>