You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuscany.apache.org by Daniel Kulp <da...@iona.com> on 2006/04/26 14:05:19 UTC
Checkstyle enforcement proposal...
Copying out of Jim's message into an appropriate titled message to make
sure it doesn't get lost:
Dan also proposed we adopt checkstyle as part of the build process. I
would like to due this using lazy consensus with the caveat that the
cut-over be done after the Java One release to avoid disruption and
give people a chance to adjust code gradually. The checkstyle
configuration is fairly rigorous and we will need to divide up the
work to do so by project.
My information:
I've attached the "proposed" checkstyle.xml. In there, there are links
to checkstyles website that describes each of the checks. All of the
checkstyle checks are listed in the file, but many are disabled. They
are left in the file so people can see what is available and we can
decide if we want them enabled or not.
There may need to be an additional change. The "beta" version of the
next Checkstyle eclipse plugin uses a beta version of checkstyle 4.2.
It adds TYPECAST to the defaults for "NoWhitespaceAfter". Thus, code
like:
Foo foo = (Foo) obj;
is marked illegal with the newer version. (the current maven version is
OK) I notice we do that in many places (although there are also a lot
of typecasts without the space). Anyway, if we want to continue
supporting both with and without spaces, that test would need to be
updated to explicitly list the tokens we DO want. However, I personally
feel we should leave it as is and remove the spaces, but that's my
preference. Unfortunately, it would be hard to enforce until checkstyle
4.2.
In general, the tests that require some discussion are:
1) AbstractClassName: what names should abstract classes have. Celtix
has a couple. I haven't looked at tuscany much in this regard.
2) AvoidStarImport and ImportOrder - Celtix DOES allow star imports for
some of the "java" packages (io, util, net, etc...) and junit. All
others cannot have start imports. We also order them more like the old
"C" ordering of "system stuff first" (java., javax. org.w3c., org.xml)
followed by all other stuff, in alpha order.
3) Lengths always start good discussions. We limit LineLength to 110
(but the attached checkstyle is set to 115), Method Length to 150, number
of parameters to a method to 7, and AnonymousInnerClasses set to 40
lines. (which seems high to me, but...)
I'd ALSO like this discussion done for the SDO and SPEC projects as well
as the generated code. But those can wait. Right now, those parts are
COMPLETELY different than the rest of the tuscany code. It's a bit
strange when debugging when you trace into that code as suddenly
everything looks different. I guess the question is: are those parts
part of the tuscany project or not. If not, should they move to a
different project? If so, shouldn't they follow the same rules?
That all said, here's my +1. :-) Definitely wait until after JavaOne.
I'd definitely be willing to help out, possibly even willing to commit to
doing a module a day.
Enjoy!
--
J. Daniel Kulp
Principal Engineer
IONA
P: 781-902-8727 C: 508-380-7194
daniel.kulp@iona.com
Re: Checkstyle enforcement proposal...
Posted by Daniel Kulp <da...@iona.com>.
Jeremy,
a) The time it takes is actually very small. It will probably end up
adding about 15 seconds to an entire build from tuscany/java.
Checkstyle is very quick. (pmd is not)
b) I can make anything we want WARNING's instead of ERRORs. Warnings
would not fail the build. I would put the hidden thing as warning (not
critical to cleanup, but we probably should consider it) as well as some
of the whitespace things.
Would that work for you?
Dan
On Wednesday 26 April 2006 18:47, Jeremy Boynes wrote:
> On 4/26/06, Daniel Kulp <da...@iona.com> wrote:
> > My information:
> > I've attached the "proposed" checkstyle.xml. In there, there are
> > links to checkstyles website that describes each of the checks. All
> > of the checkstyle checks are listed in the file, but many are
> > disabled. They are left in the file so people can see what is
> > available and we can decide if we want them enabled or not.
>
> -0
>
> I like the idea in theory but have concerns with:
> a) the time it takes to run the plugin especially with the number of
> modules we have
> is there way it can be run pre-commit?
> b) failing the build due to a missing, insignificant space seems harsh
> especially
> when there are other more worrying issues like missing tests
>
> Call me naive but I would hope self-discipline (and "reformat code" in
> the IDE) would keep layout issues under control and that unit testing
> (backed by application of a large fish) would keep bugs under control.
>
> --
> Jeremy
--
J. Daniel Kulp
Principal Engineer
IONA
P: 781-902-8727 C: 508-380-7194
daniel.kulp@iona.com
Re: Checkstyle enforcement proposal...
Posted by Jim Marino <ji...@gmail.com>.
Yep, absolutely that makes sense.
Jim
On Apr 26, 2006, at 7:03 PM, Jean-Sebastien Delfino wrote:
> Jim Marino wrote:
>
>> I agree poor test coverage is more important but this shouldn't be
>> a big deal - I'll even do it for all of the packages myself. I
>> think this is one of those incremental improvements that will make
>> the code more consistent and easier for people to understand
>> coming fresh into the project. I'm also proposing this for after
>> JavaOne and many of the more important items.
>>
>> Jim
>>
>> On Apr 26, 2006, at 4:15 PM, Jeremy Boynes wrote:
>>
>>
>>> Jim Marino wrote:
>>>
>>>
>>>> So if we do not have the space restriction, checks for parameter
>>>> names, and run this only pre-commit, would you be o.k. with it?
>>>> I would like to have this in since it is a nice check and should
>>>> not be burdensome assuming people set the proper template in
>>>> their IDE.
>>>>
>>>>
>>>
>>> Doing it that way would get me to +0 - I just think we're
>>> focusing a little much on layout and not enough on e.g. testing.
>>>
>>> I would like to hear what others have to say.
>>> --
>>> Jeremy
>>>
>>>
>>>
>>
>>
>>
>
> I'm +1 on the goal to enable checkstyle.
> I'm also +1 on Dan's initial proposal which was to do this *after*
> JavaOne.
>
> Since most of us are busy now with testing and/or critical fixes
> for our JavaOne release, I suggest that we try to sort out the
> details of the style, spacing, typecasting, checking for parameter
> names etc. after the release. Does that make sense?
>
> --
> Jean-Sebastien
>
>
Re: Checkstyle enforcement proposal...
Posted by Jean-Sebastien Delfino <js...@apache.org>.
Jim Marino wrote:
> I agree poor test coverage is more important but this shouldn't be a
> big deal - I'll even do it for all of the packages myself. I think
> this is one of those incremental improvements that will make the code
> more consistent and easier for people to understand coming fresh into
> the project. I'm also proposing this for after JavaOne and many of the
> more important items.
>
> Jim
>
> On Apr 26, 2006, at 4:15 PM, Jeremy Boynes wrote:
>
>> Jim Marino wrote:
>>
>>> So if we do not have the space restriction, checks for parameter
>>> names, and run this only pre-commit, would you be o.k. with it? I
>>> would like to have this in since it is a nice check and should not
>>> be burdensome assuming people set the proper template in their IDE.
>>>
>>
>> Doing it that way would get me to +0 - I just think we're focusing a
>> little much on layout and not enough on e.g. testing.
>>
>> I would like to hear what others have to say.
>> --
>> Jeremy
>>
>>
>
>
I'm +1 on the goal to enable checkstyle.
I'm also +1 on Dan's initial proposal which was to do this *after* JavaOne.
Since most of us are busy now with testing and/or critical fixes for our
JavaOne release, I suggest that we try to sort out the details of the
style, spacing, typecasting, checking for parameter names etc. after
the release. Does that make sense?
--
Jean-Sebastien
Re: Checkstyle enforcement proposal...
Posted by Jim Marino <ji...@gmail.com>.
I agree poor test coverage is more important but this shouldn't be a
big deal - I'll even do it for all of the packages myself. I think
this is one of those incremental improvements that will make the code
more consistent and easier for people to understand coming fresh into
the project. I'm also proposing this for after JavaOne and many of
the more important items.
Jim
On Apr 26, 2006, at 4:15 PM, Jeremy Boynes wrote:
> Jim Marino wrote:
>
>> So if we do not have the space restriction, checks for parameter
>> names, and run this only pre-commit, would you be o.k. with it? I
>> would like to have this in since it is a nice check and should not
>> be burdensome assuming people set the proper template in their IDE.
>>
>
> Doing it that way would get me to +0 - I just think we're focusing
> a little much on layout and not enough on e.g. testing.
>
> I would like to hear what others have to say.
> --
> Jeremy
>
>
Re: Checkstyle enforcement proposal...
Posted by Jeremy Boynes <jb...@apache.org>.
Jim Marino wrote:
> So if we do not have the space restriction, checks for parameter names,
> and run this only pre-commit, would you be o.k. with it? I would like
> to have this in since it is a nice check and should not be burdensome
> assuming people set the proper template in their IDE.
>
Doing it that way would get me to +0 - I just think we're focusing a
little much on layout and not enough on e.g. testing.
I would like to hear what others have to say.
--
Jeremy
Re: Checkstyle enforcement proposal...
Posted by Jim Marino <jm...@myromatours.com>.
So if we do not have the space restriction, checks for parameter
names, and run this only pre-commit, would you be o.k. with it? I
would like to have this in since it is a nice check and should not be
burdensome assuming people set the proper template in their IDE.
On Apr 26, 2006, at 3:47 PM, Jeremy Boynes wrote:
> On 4/26/06, Daniel Kulp <da...@iona.com> wrote:
>
>> My information:
>> I've attached the "proposed" checkstyle.xml. In there, there
>> are links
>> to checkstyles website that describes each of the checks. All of
>> the
>> checkstyle checks are listed in the file, but many are disabled.
>> They
>> are left in the file so people can see what is available and we can
>> decide if we want them enabled or not.
>>
>>
>
> -0
>
> I like the idea in theory but have concerns with:
> a) the time it takes to run the plugin especially with the number of
> modules we have
> is there way it can be run pre-commit?
> b) failing the build due to a missing, insignificant space seems harsh
> especially
> when there are other more worrying issues like missing tests
>
> Call me naive but I would hope self-discipline (and "reformat code" in
> the IDE) would keep layout issues under control and that unit testing
> (backed by application of a large fish) would keep bugs under control.
>
> --
> Jeremy
>
Re: Checkstyle enforcement proposal...
Posted by Jeremy Boynes <jb...@apache.org>.
On 4/26/06, Daniel Kulp <da...@iona.com> wrote:
> My information:
> I've attached the "proposed" checkstyle.xml. In there, there are links
> to checkstyles website that describes each of the checks. All of the
> checkstyle checks are listed in the file, but many are disabled. They
> are left in the file so people can see what is available and we can
> decide if we want them enabled or not.
>
-0
I like the idea in theory but have concerns with:
a) the time it takes to run the plugin especially with the number of
modules we have
is there way it can be run pre-commit?
b) failing the build due to a missing, insignificant space seems harsh
especially
when there are other more worrying issues like missing tests
Call me naive but I would hope self-discipline (and "reformat code" in
the IDE) would keep layout issues under control and that unit testing
(backed by application of a large fish) would keep bugs under control.
--
Jeremy
Re: Checkstyle enforcement proposal...
Posted by Jeremy Boynes <jb...@gmail.com>.
On 4/29/06, Jim Marino <jm...@myromatours.com> wrote:
> Since there are no longer -1s on the checkstyle issue, I would like
> to go ahead with implementing it after JavaOne. Jeremy, I assume you
> are o.k. with the proposal now on the table?
>
+0 (and I think we have consensus that we do this only after JavaOne)
--
Jeremy
Re: Checkstyle enforcement proposal...
Posted by Jim Marino <jm...@myromatours.com>.
On Apr 29, 2006, at 1:03 AM, ant elder wrote:
> On 4/26/06, Jim Marino <jm...@myromatours.com> wrote:
>
> <snip/>
>
> Ant, you -1 this stating that you weren't sure what this entails and
>
>> that it was buried in another email. Now that Dan pulled it out and
>> included a description of what checkstyle is, do you have specific
>> concerns about the proposal?
>>
>
>
> For the record yes I'm no longer -1. I do agree with Dan that having a
> consistent style does make code look better and makes it easier to
> understand. It does seem a bit odd that we looking at having bad
> style fail
> a build and checking this with every build but at the same time
> still don't
> test things that really matter like the samples.
I believe there is violent agreement that our test coverage is
lacking (sucks). It was decided a while back that this should be
fixed by writing test cases which exercise our APIs and integration
with deployment platforms. I don't think we need to continue to
rehash this - let's just go about doing it.
> Another concern i have is I
> expect this will cause more build breaks - I'm fine with that,
> builds break
> - but right now people seem to be taking build breaks a bit more
> seriously
> than I'd like.
I'm not sure how it will cause more build breaks except on
developers' machines when they perform a build prior to checkin. I
think this is a good thing since it will stop code from being checked
in that shouldn't be.
Could you be more specific about your concern over the seriousness of
build breaks? Our policy has been that when they happen, the person
responsible needs to fix it ASAP since it holds everyone else up.
There was an occasion when we purposely decided to break the build
for two days, but that was a deliberate exception and discussed on
the list prior to it being done. Once we have an integration server
set up, this means that developers will need to wait around five
minutes or so after a checkin to receive an all-clear email. I'm not
concerned about this as it is standard practice, are you?
Since there are no longer -1s on the checkstyle issue, I would like
to go ahead with implementing it after JavaOne. Jeremy, I assume you
are o.k. with the proposal now on the table?
Jim
>
> ...ant
>
Re: Checkstyle enforcement proposal...
Posted by ant elder <an...@gmail.com>.
On 4/26/06, Jim Marino <jm...@myromatours.com> wrote:
<snip/>
Ant, you -1 this stating that you weren't sure what this entails and
> that it was buried in another email. Now that Dan pulled it out and
> included a description of what checkstyle is, do you have specific
> concerns about the proposal?
For the record yes I'm no longer -1. I do agree with Dan that having a
consistent style does make code look better and makes it easier to
understand. It does seem a bit odd that we looking at having bad style fail
a build and checking this with every build but at the same time still don't
test things that really matter like the samples. Another concern i have is I
expect this will cause more build breaks - I'm fine with that, builds break
- but right now people seem to be taking build breaks a bit more seriously
than I'd like.
...ant
Re: Checkstyle enforcement proposal...
Posted by Jim Marino <jm...@myromatours.com>.
The only one I think we may not want is the TYPECAST. That said, I'm
not fussy about line length as long as it is consistent, not too
long, not too short. One thing I would like to change is throwing
errors for when parameter names hide member variables since I think
this is o.k. Dan, do you have an objection to removing that check?
Ant, you -1 this stating that you weren't sure what this entails and
that it was buried in another email. Now that Dan pulled it out and
included a description of what checkstyle is, do you have specific
concerns about the proposal?
Others may have specific objections as well. Please speak up, for or
against :-)
Jim
On Apr 26, 2006, at 6:39 AM, Daniel Kulp wrote:
>
>
> One more comment:
> One complaint I've heard from developers about enabling checkstyle is:
>
> It will (a) slow me down because (b) it doesn't match my style.
>
>
> a) Based on my experience on several projects where checkstyle was
> added,
> it's really not true in the long term. For the first week or so, it
> DOES slow people down a bit, but once developers started getting
> used to
> the style, got their IDE's setup, etc... it no longer is a
> problem. If
> you ask any of the Celtix developers, most of them don't even realize
> checkstyle is even there anymore. They've gotten used to the
> style so
> it's rare that they even hit it. Long term, and with larger
> teams
> (Celtix has about 14 active commiters and growing), it actually
> speeds up
> the development as debugging and tracing and reading other peoples
> code
> becomes much easier as it's all 100% consistent from a style
> standpoint.
> (there are still inconsistencies in algorithms and approaches and
> such,
> obviously, but I haven't found a tool for that yet. :-)
>
> b) That's the point. Consistency across an entire project is
> important
> as it speeds up the developers. See point (a). Your not conforming
>
> Anyway, I kind of relate enabling checkstyle to be similar to
> switching to
> a kinesis contoured keyboard or other ergo keyboard.
> (http://www.kinesis-ergo.com/contoured_usb.htm , highly
> recommended BTW)
> For the first week, you WILL be slower, but long term, there are a
> lot of
> good things that come out of it.
>
> Enjoy!
> Dan
>
>
>
> On Wednesday 26 April 2006 08:05, Daniel Kulp wrote:
>
>> Copying out of Jim's message into an appropriate titled message to
>> make
>> sure it doesn't get lost:
>>
>> Dan also proposed we adopt checkstyle as part of the build process. I
>> would like to due this using lazy consensus with the caveat that the
>> cut-over be done after the Java One release to avoid disruption and
>> give people a chance to adjust code gradually. The checkstyle
>> configuration is fairly rigorous and we will need to divide up the
>> work to do so by project.
>>
>>
>> My information:
>> I've attached the "proposed" checkstyle.xml. In there, there are
>> links to checkstyles website that describes each of the checks. All
>> of the checkstyle checks are listed in the file, but many are
>> disabled.
>> They are left in the file so people can see what is available and we
>> can decide if we want them enabled or not.
>>
>> There may need to be an additional change. The "beta" version of
>> the
>> next Checkstyle eclipse plugin uses a beta version of checkstyle 4.2.
>> It adds TYPECAST to the defaults for "NoWhitespaceAfter". Thus,
>> code
>> like:
>> Foo foo = (Foo) obj;
>> is marked illegal with the newer version. (the current maven
>> version
>> is OK) I notice we do that in many places (although there are
>> also a
>> lot of typecasts without the space). Anyway, if we want to
>> continue supporting both with and without spaces, that test would
>> need
>> to be updated to explicitly list the tokens we DO want. However, I
>> personally feel we should leave it as is and remove the spaces, but
>> that's my preference. Unfortunately, it would be hard to enforce
>> until checkstyle 4.2.
>>
>> In general, the tests that require some discussion are:
>> 1) AbstractClassName: what names should abstract classes have.
>> Celtix
>> has a couple. I haven't looked at tuscany much in this regard.
>>
>> 2) AvoidStarImport and ImportOrder - Celtix DOES allow star
>> imports for
>> some of the "java" packages (io, util, net, etc...) and junit. All
>> others cannot have start imports. We also order them more like the
>> old "C" ordering of "system stuff first" (java., javax. org.w3c.,
>> org.xml) followed by all other stuff, in alpha order.
>>
>> 3) Lengths always start good discussions. We limit LineLength to
>> 110
>> (but the attached checkstyle is set to 115), Method Length to 150,
>> number of parameters to a method to 7, and AnonymousInnerClasses
>> set to
>> 40 lines. (which seems high to me, but...)
>>
>> I'd ALSO like this discussion done for the SDO and SPEC projects as
>> well as the generated code. But those can wait. Right now, those
>> parts are COMPLETELY different than the rest of the tuscany code.
>> It's a bit strange when debugging when you trace into that code as
>> suddenly everything looks different. I guess the question is: are
>> those parts part of the tuscany project or not. If not, should they
>> move to a different project? If so, shouldn't they follow the same
>> rules?
>>
>> That all said, here's my +1. :-) Definitely wait until after
>> JavaOne. I'd definitely be willing to help out, possibly even willing
>> to commit to doing a module a day.
>>
>> Enjoy!
>>
>
> --
> J. Daniel Kulp
> Principal Engineer
> IONA
> P: 781-902-8727 C: 508-380-7194
> daniel.kulp@iona.com
>
Re: Checkstyle enforcement proposal...
Posted by Daniel Kulp <da...@iona.com>.
One more comment:
One complaint I've heard from developers about enabling checkstyle is:
It will (a) slow me down because (b) it doesn't match my style.
a) Based on my experience on several projects where checkstyle was added,
it's really not true in the long term. For the first week or so, it
DOES slow people down a bit, but once developers started getting used to
the style, got their IDE's setup, etc... it no longer is a problem. If
you ask any of the Celtix developers, most of them don't even realize
checkstyle is even there anymore. They've gotten used to the style so
it's rare that they even hit it. Long term, and with larger teams
(Celtix has about 14 active commiters and growing), it actually speeds up
the development as debugging and tracing and reading other peoples code
becomes much easier as it's all 100% consistent from a style standpoint.
(there are still inconsistencies in algorithms and approaches and such,
obviously, but I haven't found a tool for that yet. :-)
b) That's the point. Consistency across an entire project is important
as it speeds up the developers. See point (a). Your not conforming
Anyway, I kind of relate enabling checkstyle to be similar to switching to
a kinesis contoured keyboard or other ergo keyboard.
(http://www.kinesis-ergo.com/contoured_usb.htm , highly recommended BTW)
For the first week, you WILL be slower, but long term, there are a lot of
good things that come out of it.
Enjoy!
Dan
On Wednesday 26 April 2006 08:05, Daniel Kulp wrote:
> Copying out of Jim's message into an appropriate titled message to make
> sure it doesn't get lost:
>
> Dan also proposed we adopt checkstyle as part of the build process. I
> would like to due this using lazy consensus with the caveat that the
> cut-over be done after the Java One release to avoid disruption and
> give people a chance to adjust code gradually. The checkstyle
> configuration is fairly rigorous and we will need to divide up the
> work to do so by project.
>
>
> My information:
> I've attached the "proposed" checkstyle.xml. In there, there are
> links to checkstyles website that describes each of the checks. All
> of the checkstyle checks are listed in the file, but many are disabled.
> They are left in the file so people can see what is available and we
> can decide if we want them enabled or not.
>
> There may need to be an additional change. The "beta" version of the
> next Checkstyle eclipse plugin uses a beta version of checkstyle 4.2.
> It adds TYPECAST to the defaults for "NoWhitespaceAfter". Thus, code
> like:
> Foo foo = (Foo) obj;
> is marked illegal with the newer version. (the current maven version
> is OK) I notice we do that in many places (although there are also a
> lot of typecasts without the space). Anyway, if we want to
> continue supporting both with and without spaces, that test would need
> to be updated to explicitly list the tokens we DO want. However, I
> personally feel we should leave it as is and remove the spaces, but
> that's my preference. Unfortunately, it would be hard to enforce
> until checkstyle 4.2.
>
> In general, the tests that require some discussion are:
> 1) AbstractClassName: what names should abstract classes have. Celtix
> has a couple. I haven't looked at tuscany much in this regard.
>
> 2) AvoidStarImport and ImportOrder - Celtix DOES allow star imports for
> some of the "java" packages (io, util, net, etc...) and junit. All
> others cannot have start imports. We also order them more like the
> old "C" ordering of "system stuff first" (java., javax. org.w3c.,
> org.xml) followed by all other stuff, in alpha order.
>
> 3) Lengths always start good discussions. We limit LineLength to 110
> (but the attached checkstyle is set to 115), Method Length to 150,
> number of parameters to a method to 7, and AnonymousInnerClasses set to
> 40 lines. (which seems high to me, but...)
>
> I'd ALSO like this discussion done for the SDO and SPEC projects as
> well as the generated code. But those can wait. Right now, those
> parts are COMPLETELY different than the rest of the tuscany code.
> It's a bit strange when debugging when you trace into that code as
> suddenly everything looks different. I guess the question is: are
> those parts part of the tuscany project or not. If not, should they
> move to a different project? If so, shouldn't they follow the same
> rules?
>
> That all said, here's my +1. :-) Definitely wait until after
> JavaOne. I'd definitely be willing to help out, possibly even willing
> to commit to doing a module a day.
>
> Enjoy!
--
J. Daniel Kulp
Principal Engineer
IONA
P: 781-902-8727 C: 508-380-7194
daniel.kulp@iona.com