+1. teh goog was pretty militant about style checking, and I would be happy to bring that process back.
I *think* we're done with major code movements as of the recent unit test/integration test split (which is delightful- thank you again Matthias.) Gabriel, it might be a good time for a format-all. J On Sat, Jul 14, 2012 at 6:17 AM, Gabriel Reid <[email protected]>wrote: > Hi Matthias, > > I'm definitely in favor of going forward with this -- consistent code > style definitely makes it easier for me (and I think most people) to read > code. > > I little while back I added an eclipse formatter profile to the project, > with the intention of all Eclipse users making use of it. As far as I know, > the formatting setup is in line with what you've outlined below, but I'll > have to double check it to see about getting around the "ctor def throws at > indentation…" warning. > > I've been planning on doing a format-all of the whole project, but I > haven't done it in the past couple of weeks because there has been so much > file moving/renaming going on, so I didn't want to get in the way of other > patches at the same time. > > - Gabriel > > > On Saturday 14 July 2012 at 11:31, Matthias Friedrich wrote: > > > Hi, > > > > some of you mentioned that you're interested in using Checkstyle > > for Crunch. I realize that people making style proposals aren't the > > most popular bunch, but I'll give it a try anyway ;-) > > > > I took a first stab at a checkstyle configuration [1] that can serve > > as a basis for discussion. It's mostly the default config for Eclipse > > tweaked to fit Crunch's existing code base. Basically, I changed the > > following things: > > > > * Only require Javadoc for types (classes/interfaces/enums), but > > eventually the entire pusblished API should be documented > > * Indent by 2 spaces instead of 4 (Indentation) > > * Set line limit from 80 to 120 characters (LineLength) > > * Don't allow tabs (FileTabCharacter) > > * Disabled DesignForExtension (we should really have this though) > > * Parameters may shadow fields (HiddenField) > > * Parameters don't need to be final (FinalParameters) > > * Allow magic numbers, otherwise it's too annoying (MagicNumers) > > > > I've created a sample report [2] to give you a first impression. > > Checkstyle flags 900 warnings for 20000 LoC which is a pretty good > > value for a project that hasn't used Checkstyle during development. > > A relatively large number of warnings is caused by a small number of > > files (mostly code indented by 4 instead of 2 spaces), so there are > > lots of quick wins. > > > > The 2-space indenting my prove to be difficult; I haven't been able to > > suppress some warnings concerning line wrapping ("ctor def throws at > > indentation ..."). But perhaps I've overlooked something. > > > > What do you think? Should we continue working on this? > > > > Regards, > > Matthias > > > > [1] http://users.mafr.de/~matthias/crunch/checkstyle.xml > > [2] http://users.mafr.de/~matthias/crunch/checkstyle/checkstyle.html > > > > -- Director of Data Science Cloudera <http://www.cloudera.com> Twitter: @josh_wills <http://twitter.com/josh_wills>
