wrt incessant nit-picking

[Adrian Cole]

I appreciate that people are reviewing change now.  It is going to take time before we strike the right balance here.  While there are some notable cases of good feedback, we've gotten into a case where feedback is generally around nits that could be caught with checkstyle.

When the issues history is cluttered with a nit-storm, it is hard to see useful feedback (similar to weeding through a spam-box).  It has been denial-of-servicing my time, and I'm sure others have similar sentiment.

Nit storms displace time, rebasing and re-reviewing, and certainly merging several backport nit requests do, too.  This time comes from somewhere whether it is out of a balance that can be used for real change, reviewing others' work, sleeping, or crazy as it sounds, doing our day jobs!

If any of you feel strongly enough to nit about things that can be in checkstyle, please focus your effort on completing checkstyle enforcement.

We all want the project better and a place to participate, but I think our time is best spent on hard-hitting points.

-A

[Everett Toews]

I think this is a reasonable point. Just because you are reviewing something, doesn't mean you *have* to find *something* wrong with it.

Everett

--
You received this message because you are subscribed to the Google Groups "jclouds-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jclouds-dev...@googlegroups.com.
To post to this group, send email to jclou...@googlegroups.com.
Visit this group at http://groups.google.com/group/jclouds-dev?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Andrew Gaul]

I agree that recently reviews have focused more on stylistic than substantive issues.  While I prefer that we focus on more important issues, we can automate and eliminate many comments via Checkstyle as you suggest.  I can integrate Checkstyle into Buildhive and clean up any remaining violations but I lack the expertise to integrate with Eclipse.  Will someone else volunteer to do this?

[Adrian Cole]

by integrate you mean have eclipse read checkstyle config, right? similar to http://jgonian.wordpress.com/2010/12/12/maven-checkstyle-and-eclipse/ updated to whatever state of the art is?

--

[Andrew Bayer]

fwiw, I am strongly pro-checkstyle - I like it when something tells me I didn't do the polite thing, shall we say, since I don't generally know what the polite thing would be. Whirr's got some pretty basic checkstyle rules, and has it integrated into the build, so that you can't actually spit out jars unless you pass the checkstyle rules. Now, obviously, that's harder to do with an established code base that probably has a lot of violations in it already...

A.

[Andrew Gaul]

I cannot comment on Eclipse state of the art, but I believe it can show violations in the editor.  I suspect most developers use Eclipse and we should make extra effort to accommodate them.  I use vim so before submitting a pull request I run mvn checkstyle:checkstyle -Dcheckstyle.output.file=/dev/stdout -Dcheckstyle.output.format=plain.  Newer versions of git can automate this with a pre-push hook.  BuildHive will eventually run mvn checkstyle:check to enforce our settings.

[Adrian Cole]

+1

ps wrt Eclipse I don't think we need to block on spoon feeding.  There is a plugin which can be manually configured to point to the right directory in worst case.

-A

[Ioannis Canellos]

If it doesn't dramatically increase the build-time, I'd love to have check-style run by default as whirr does.

Ioannis Canellos

Blog: http://iocanel.blogspot.com

Twitter: iocanel

[Andrew Phillips]

>> should make extra effort to accommodate them. I use vim so before
>> submitting a pull request I run mvn checkstyle:checkstyle
>> -Dcheckstyle.output.file=/dev/stdout
>> -Dcheckstyle.output.format=plain.

Will see if we can get this hooked up into CloudBees, too.

ap

[Adrian Cole]

absolutely.  thanks in advance for chasing this down, Andrew P.

ap

--
You received this message because you are subscribed to the Google Groups "jclouds-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jclouds-dev+unsubscribe@googlegroups.com.

[Andrew Phillips]

Quoting Adrian Cole <adrian...@gmail.com>:

> absolutely. thanks in advance for chasing this down, Andrew P.

The pull request builder job now runs Checkstyle (the Violations
plugin could also run PMD, Findbugs and others should we ever be
interested in those). There's a chart on the build overview page [1]
and a link to the detailed report [2].

Unfortunately, I can't see an easy way yet to jump to violations
related to the pull request in question, but the overview graphic will
at least show whether any new violations have been added.

We can also configure the "sunny/stormy/unstable" boundaries; they're
currently at the default values of:

Sunny: 0-10
Cloudy: 11-999
Stormy: 999+
Unstable: 999+

These are applied per module, not to the project as a whole. Any
suggestions for better values or other ways in which we could make
violations for the *PR code* easier to find much appreciated!

ap

[1] e.g.
https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/370/
[2] e.g.
https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/370/violations/