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 <[email protected]> wrote: > Why v1.5 instead of the latest? > > On Wed, Dec 24, 2014 at 3:34 PM, Christopher <[email protected]> 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 <[email protected]> > 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<[email protected]> > >>> 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<[email protected]> > >>>> 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 > >>>>> > >>>>> > >>>> > >>> >
