You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@crunch.apache.org by Matthias Friedrich <ma...@mafr.de> on 2012/07/14 11:31:22 UTC
Checkstyle config
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
Re: Checkstyle config
Posted by Matthias Friedrich <ma...@mafr.de>.
Hi,
On Saturday, 2012-07-14, Josh Wills wrote:
> +1. teh goog was pretty militant about style checking, and I would be happy
> to bring that process back.
Great, I can definitely help with that. We use CI with all kinds of
static code analysis tools at work and it definitely helped us improve
quality. It takes a while to select meaningful metrics though.
I've seen Apache provides Jenkins and Sonar instances that we can
use. Next steps would be to make Crunch ready for CI. Right now Crunch
is no good CI citizen, it leaves stuff behind in /tmp and I think the
HBase test also opens network ports.
> 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.
Glad you like it and sorry for the broken patch. I'm still new to this
git business, so if my patches don't apply cleanly just tell me and
I'll fix them.
Regards,
Matthias
Re: Checkstyle config
Posted by Gabriel Reid <ga...@gmail.com>.
Ok, sounds good, I'll go for the format-all now.
Also -- "mvn test" running in just few seconds is *awesome*, thanks Matthias!
On Saturday 14 July 2012 at 19:41, Josh Wills wrote:
> +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 <gabriel.reid@gmail.com (mailto:gabriel.reid@gmail.com)>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>
Re: Checkstyle config
Posted by Josh Wills <jw...@cloudera.com>.
+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 <ga...@gmail.com>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>
Re: Checkstyle config
Posted by Gabriel Reid <ga...@gmail.com>.
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