You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Hugo Trippaers <hu...@trippaers.nl> on 2013/11/04 15:29:50 UTC
checkstyle
Hey,
Just added a very basic checkstyle configuration to maven. The configuration file is in parents/checkstyle and it checks just a few very basic things, like trailing whitespace and tabs where there should be spaces.
I’ve enabled it for a single plugin to just the impact on build time and the amount of generated errors. Quite considerable, but i hope other parts of the code are better ;-)
You can enable check style for your plugin by adding the following to your build plugins config in maven:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>${cs.checkstyle.version}</version>
<dependencies>
<dependency>
<groupId>org.apache.cloudstack</groupId>
<artifactId>checkstyle</artifactId>
<version>0.0.1-SNAPSHOT</version>
</dependency>
</dependencies>
<executions>
<execution>
<phase>process-sources</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<failsOnError>true</failsOnError>
<configLocation>tooling/checkstyle.xml</configLocation>
<consoleOutput>true</consoleOutput>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
<sourceDirectory>${project.basedir}</sourceDirectory>
<includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
<excludes>**\/target\/,**\/bin\/</excludes>
</configuration>
</plugin>
For now its voluntary, but i would like your opinion on making this a mandatory part of the build process. Meaning a compile with not succeed when check style reports errors.
Cheers,
Hugo
Re: checkstyle
Posted by John Kinsella <jl...@stratosec.co>.
On Nov 4, 2013, at 7:54 AM, Chip Childers <ch...@apache.org> wrote:
> On Mon, Nov 04, 2013 at 04:33:07PM +0100, Hugo Trippaers wrote:
>> Hey John,
>>
>> That would be my idea.
>>
>> Make it mandatory for new (maven) projects coming into the code base and slowly start working on fixing the existing projects. The current checkstyle setting is very relaxed, just a few basic checks. Stuff that we could technically fix with a few well written regular expressions even. Over time we can start implementing parts of our code style in the checkstyle config, but that is long term planning.
>>
>> Cheers,
>>
>> Hugo
>
> We've had people correct the whitespace issues in bulk previously, but
> then someone does a merge and it goes to hell again.
>
> I'd actually be +1 on a quick fix of the code, then the enforcement of
> the rules. That is... only for the things that can be easily
> regex-fixed.
Makes sense I guess, +1 from me too.
Only thing that's really nails-on-chalkboard to me is fixing formatting and code changes in the same commit, but I haven't seen that in ACS…
Re: checkstyle
Posted by Chip Childers <ch...@apache.org>.
On Mon, Nov 04, 2013 at 04:33:07PM +0100, Hugo Trippaers wrote:
> Hey John,
>
> That would be my idea.
>
> Make it mandatory for new (maven) projects coming into the code base and slowly start working on fixing the existing projects. The current checkstyle setting is very relaxed, just a few basic checks. Stuff that we could technically fix with a few well written regular expressions even. Over time we can start implementing parts of our code style in the checkstyle config, but that is long term planning.
>
> Cheers,
>
> Hugo
We've had people correct the whitespace issues in bulk previously, but
then someone does a merge and it goes to hell again.
I'd actually be +1 on a quick fix of the code, then the enforcement of
the rules. That is... only for the things that can be easily
regex-fixed.
-chip
RE: checkstyle
Posted by Donal Lafferty <do...@citrix.com>.
Hi Laszlo,
Try building only the maven project you've modified. That will identify problems with your code much more quickly.
If formatting is a specific problem, launch checkstyle directly: there's a plugin for Eclipse (see http://dlafferty.blogspot.co.uk/2013/07/apache-cloudstack-java-coding.html)
DL
> -----Original Message-----
> From: Laszlo Hornyak [mailto:laszlo.hornyak@gmail.com]
> Sent: 04 November 2013 17:49
> To: dev@cloudstack.apache.org
> Subject: Re: checkstyle
>
> Hi,
>
> It is great to have a code standard, it is even better when it is maintained, but
> when checkstyle reports error after several minutes building the code, while
> the code should be ok and it reports an issue as important as a trailing white
> space, then I believe anyone would more likely call it a pain in the A than a
> useful tool.
> What about a non-enforcing configuration, or activation only in a profile?
> And then jenkins could send the usual error messages when it breaks, and
> we can fix it later.
>
>
>
>
> On Mon, Nov 4, 2013 at 4:33 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>
> > Hey John,
> >
> > That would be my idea.
> >
> > Make it mandatory for new (maven) projects coming into the code base
> > and slowly start working on fixing the existing projects. The current
> > checkstyle setting is very relaxed, just a few basic checks. Stuff
> > that we could technically fix with a few well written regular expressions
> even.
> > Over time we can start implementing parts of our code style in the
> > checkstyle config, but that is long term planning.
> >
> > Cheers,
> >
> > Hugo
> >
> > On 4 nov. 2013, at 16:28, John Kinsella <jl...@stratosec.co> wrote:
> >
> > > I think it'd be fairly painful to make it mandatory - maybe see if
> > > we
> > can set that as a goal for 6 months out?
> > >
> > > On Nov 4, 2013, at 6:29 AM, Hugo Trippaers <hugo@trippaers.nl<mailto:
> > hugo@trippaers.nl>>
> > > wrote:
> > >
> > > Hey,
> > >
> > > Just added a very basic checkstyle configuration to maven. The
> > configuration file is in parents/checkstyle and it checks just a few
> > very basic things, like trailing whitespace and tabs where there
> > should be spaces.
> > >
> > > I've enabled it for a single plugin to just the impact on build time
> > > and
> > the amount of generated errors. Quite considerable, but i hope other
> > parts of the code are better ;-)
> > >
> > > You can enable check style for your plugin by adding the following
> > > to
> > your build plugins config in maven:
> > >
> > > <plugin>
> > > <groupId>org.apache.maven.plugins</groupId>
> > > <artifactId>maven-checkstyle-plugin</artifactId>
> > > <version>${cs.checkstyle.version}</version>
> > > <dependencies>
> > > <dependency>
> > > <groupId>org.apache.cloudstack</groupId>
> > > <artifactId>checkstyle</artifactId>
> > > <version>0.0.1-SNAPSHOT</version>
> > > </dependency>
> > > </dependencies>
> > > <executions>
> > > <execution>
> > > <phase>process-sources</phase>
> > > <goals>
> > > <goal>check</goal>
> > > </goals>
> > > </execution>
> > > </executions>
> > > <configuration>
> > > <failsOnError>true</failsOnError>
> > > <configLocation>tooling/checkstyle.xml</configLocation>
> > > <consoleOutput>true</consoleOutput>
> > > <includeTestSourceDirectory>true</includeTestSourceDirectory>
> > > <sourceDirectory>${project.basedir}</sourceDirectory>
> > >
> > <includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
> > > <excludes>**\/target\/,**\/bin\/</excludes>
> > > </configuration>
> > > </plugin>
> > >
> > >
> > > For now its voluntary, but i would like your opinion on making this
> > > a
> > mandatory part of the build process. Meaning a compile with not
> > succeed when check style reports errors.
> > >
> > > Cheers,
> > >
> > > Hugo
> > >
> > > Stratosec<http://stratosec.co/> - Compliance as a Service
> > > o: 415.315.9385
> > > @johnlkinsella<http://twitter.com/johnlkinsella>
> > >
> >
> >
>
>
> --
>
> EOF
Re: checkstyle
Posted by Laszlo Hornyak <la...@gmail.com>.
Hi,
It is great to have a code standard, it is even better when it is
maintained, but when checkstyle reports error after several minutes
building the code, while the code should be ok and it reports an issue as
important as a trailing white space, then I believe anyone would more
likely call it a pain in the A than a useful tool.
What about a non-enforcing configuration, or activation only in a profile?
And then jenkins could send the usual error messages when it breaks, and we
can fix it later.
On Mon, Nov 4, 2013 at 4:33 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
> Hey John,
>
> That would be my idea.
>
> Make it mandatory for new (maven) projects coming into the code base and
> slowly start working on fixing the existing projects. The current
> checkstyle setting is very relaxed, just a few basic checks. Stuff that we
> could technically fix with a few well written regular expressions even.
> Over time we can start implementing parts of our code style in the
> checkstyle config, but that is long term planning.
>
> Cheers,
>
> Hugo
>
> On 4 nov. 2013, at 16:28, John Kinsella <jl...@stratosec.co> wrote:
>
> > I think it'd be fairly painful to make it mandatory - maybe see if we
> can set that as a goal for 6 months out?
> >
> > On Nov 4, 2013, at 6:29 AM, Hugo Trippaers <hugo@trippaers.nl<mailto:
> hugo@trippaers.nl>>
> > wrote:
> >
> > Hey,
> >
> > Just added a very basic checkstyle configuration to maven. The
> configuration file is in parents/checkstyle and it checks just a few very
> basic things, like trailing whitespace and tabs where there should be
> spaces.
> >
> > I’ve enabled it for a single plugin to just the impact on build time and
> the amount of generated errors. Quite considerable, but i hope other parts
> of the code are better ;-)
> >
> > You can enable check style for your plugin by adding the following to
> your build plugins config in maven:
> >
> > <plugin>
> > <groupId>org.apache.maven.plugins</groupId>
> > <artifactId>maven-checkstyle-plugin</artifactId>
> > <version>${cs.checkstyle.version}</version>
> > <dependencies>
> > <dependency>
> > <groupId>org.apache.cloudstack</groupId>
> > <artifactId>checkstyle</artifactId>
> > <version>0.0.1-SNAPSHOT</version>
> > </dependency>
> > </dependencies>
> > <executions>
> > <execution>
> > <phase>process-sources</phase>
> > <goals>
> > <goal>check</goal>
> > </goals>
> > </execution>
> > </executions>
> > <configuration>
> > <failsOnError>true</failsOnError>
> > <configLocation>tooling/checkstyle.xml</configLocation>
> > <consoleOutput>true</consoleOutput>
> > <includeTestSourceDirectory>true</includeTestSourceDirectory>
> > <sourceDirectory>${project.basedir}</sourceDirectory>
> >
> <includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
> > <excludes>**\/target\/,**\/bin\/</excludes>
> > </configuration>
> > </plugin>
> >
> >
> > For now its voluntary, but i would like your opinion on making this a
> mandatory part of the build process. Meaning a compile with not succeed
> when check style reports errors.
> >
> > Cheers,
> >
> > Hugo
> >
> > Stratosec<http://stratosec.co/> - Compliance as a Service
> > o: 415.315.9385
> > @johnlkinsella<http://twitter.com/johnlkinsella>
> >
>
>
--
EOF
Re: checkstyle
Posted by Hugo Trippaers <hu...@trippaers.nl>.
Hey John,
That would be my idea.
Make it mandatory for new (maven) projects coming into the code base and slowly start working on fixing the existing projects. The current checkstyle setting is very relaxed, just a few basic checks. Stuff that we could technically fix with a few well written regular expressions even. Over time we can start implementing parts of our code style in the checkstyle config, but that is long term planning.
Cheers,
Hugo
On 4 nov. 2013, at 16:28, John Kinsella <jl...@stratosec.co> wrote:
> I think it'd be fairly painful to make it mandatory - maybe see if we can set that as a goal for 6 months out?
>
> On Nov 4, 2013, at 6:29 AM, Hugo Trippaers <hu...@trippaers.nl>>
> wrote:
>
> Hey,
>
> Just added a very basic checkstyle configuration to maven. The configuration file is in parents/checkstyle and it checks just a few very basic things, like trailing whitespace and tabs where there should be spaces.
>
> I’ve enabled it for a single plugin to just the impact on build time and the amount of generated errors. Quite considerable, but i hope other parts of the code are better ;-)
>
> You can enable check style for your plugin by adding the following to your build plugins config in maven:
>
> <plugin>
> <groupId>org.apache.maven.plugins</groupId>
> <artifactId>maven-checkstyle-plugin</artifactId>
> <version>${cs.checkstyle.version}</version>
> <dependencies>
> <dependency>
> <groupId>org.apache.cloudstack</groupId>
> <artifactId>checkstyle</artifactId>
> <version>0.0.1-SNAPSHOT</version>
> </dependency>
> </dependencies>
> <executions>
> <execution>
> <phase>process-sources</phase>
> <goals>
> <goal>check</goal>
> </goals>
> </execution>
> </executions>
> <configuration>
> <failsOnError>true</failsOnError>
> <configLocation>tooling/checkstyle.xml</configLocation>
> <consoleOutput>true</consoleOutput>
> <includeTestSourceDirectory>true</includeTestSourceDirectory>
> <sourceDirectory>${project.basedir}</sourceDirectory>
> <includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
> <excludes>**\/target\/,**\/bin\/</excludes>
> </configuration>
> </plugin>
>
>
> For now its voluntary, but i would like your opinion on making this a mandatory part of the build process. Meaning a compile with not succeed when check style reports errors.
>
> Cheers,
>
> Hugo
>
> Stratosec<http://stratosec.co/> - Compliance as a Service
> o: 415.315.9385
> @johnlkinsella<http://twitter.com/johnlkinsella>
>
Re: checkstyle
Posted by John Kinsella <jl...@stratosec.co>.
I think it'd be fairly painful to make it mandatory - maybe see if we can set that as a goal for 6 months out?
On Nov 4, 2013, at 6:29 AM, Hugo Trippaers <hu...@trippaers.nl>>
wrote:
Hey,
Just added a very basic checkstyle configuration to maven. The configuration file is in parents/checkstyle and it checks just a few very basic things, like trailing whitespace and tabs where there should be spaces.
I’ve enabled it for a single plugin to just the impact on build time and the amount of generated errors. Quite considerable, but i hope other parts of the code are better ;-)
You can enable check style for your plugin by adding the following to your build plugins config in maven:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>${cs.checkstyle.version}</version>
<dependencies>
<dependency>
<groupId>org.apache.cloudstack</groupId>
<artifactId>checkstyle</artifactId>
<version>0.0.1-SNAPSHOT</version>
</dependency>
</dependencies>
<executions>
<execution>
<phase>process-sources</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<failsOnError>true</failsOnError>
<configLocation>tooling/checkstyle.xml</configLocation>
<consoleOutput>true</consoleOutput>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
<sourceDirectory>${project.basedir}</sourceDirectory>
<includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
<excludes>**\/target\/,**\/bin\/</excludes>
</configuration>
</plugin>
For now its voluntary, but i would like your opinion on making this a mandatory part of the build process. Meaning a compile with not succeed when check style reports errors.
Cheers,
Hugo
Stratosec<http://stratosec.co/> - Compliance as a Service
o: 415.315.9385
@johnlkinsella<http://twitter.com/johnlkinsella>
RE: checkstyle
Posted by Donal Lafferty <do...@citrix.com>.
I'm not a committer, so it was easiest to post a rules file on the cwiki
There's a link from the blog post http://dlafferty.blogspot.co.uk/2013/07/apache-cloudstack-java-coding.html
Direct link: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Developer+Tools
;)
DL
> -----Original Message-----
> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> Sent: 04 November 2013 16:54
> To: dev@cloudstack.apache.org
> Subject: Re: checkstyle
>
> The search-fu is weak in this one..
>
> Did you ever get to commit that file Donal? I'd be very much interested :-)
>
> Cheers,
>
> Hugo
>
> On 4 nov. 2013, at 17:48, Donal Lafferty <do...@citrix.com> wrote:
>
> > How does it compare to previous versions for CloudStack? E.g.
> > http://markmail.org/message/yz6qa2v47cdeic4d
> >
> >
> >> -----Original Message-----
> >> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> >> Sent: 04 November 2013 14:30
> >> To: dev@cloudstack.apache.org
> >> Subject: checkstyle
> >>
> >> Hey,
> >>
> >> Just added a very basic checkstyle configuration to maven. The
> >> configuration file is in parents/checkstyle and it checks just a few
> >> very basic things, like trailing whitespace and tabs where there should be
> spaces.
> >>
> >> I've enabled it for a single plugin to just the impact on build time
> >> and the amount of generated errors. Quite considerable, but i hope
> >> other parts of the code are better ;-)
> >>
> >> You can enable check style for your plugin by adding the following to
> >> your build plugins config in maven:
> >>
> >> <plugin>
> >> <groupId>org.apache.maven.plugins</groupId>
> >> <artifactId>maven-checkstyle-plugin</artifactId>
> >> <version>${cs.checkstyle.version}</version>
> >> <dependencies>
> >> <dependency>
> >> <groupId>org.apache.cloudstack</groupId>
> >> <artifactId>checkstyle</artifactId>
> >> <version>0.0.1-SNAPSHOT</version>
> >> </dependency>
> >> </dependencies>
> >> <executions>
> >> <execution>
> >> <phase>process-sources</phase>
> >> <goals>
> >> <goal>check</goal>
> >> </goals>
> >> </execution>
> >> </executions>
> >> <configuration>
> >> <failsOnError>true</failsOnError>
> >> <configLocation>tooling/checkstyle.xml</configLocation>
> >> <consoleOutput>true</consoleOutput>
> >> <includeTestSourceDirectory>true</includeTestSourceDirectory>
> >> <sourceDirectory>${project.basedir}</sourceDirectory>
> >>
> >> <includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
> >> <excludes>**\/target\/,**\/bin\/</excludes>
> >> </configuration>
> >> </plugin>
> >>
> >>
> >> For now its voluntary, but i would like your opinion on making this a
> >> mandatory part of the build process. Meaning a compile with not
> >> succeed when check style reports errors.
> >>
> >> Cheers,
> >>
> >> Hugo
Re: checkstyle
Posted by Hugo Trippaers <hu...@trippaers.nl>.
The search-fu is weak in this one..
Did you ever get to commit that file Donal? I’d be very much interested :-)
Cheers,
Hugo
On 4 nov. 2013, at 17:48, Donal Lafferty <do...@citrix.com> wrote:
> How does it compare to previous versions for CloudStack? E.g. http://markmail.org/message/yz6qa2v47cdeic4d
>
>
>> -----Original Message-----
>> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
>> Sent: 04 November 2013 14:30
>> To: dev@cloudstack.apache.org
>> Subject: checkstyle
>>
>> Hey,
>>
>> Just added a very basic checkstyle configuration to maven. The configuration
>> file is in parents/checkstyle and it checks just a few very basic things, like
>> trailing whitespace and tabs where there should be spaces.
>>
>> I've enabled it for a single plugin to just the impact on build time and the
>> amount of generated errors. Quite considerable, but i hope other parts of
>> the code are better ;-)
>>
>> You can enable check style for your plugin by adding the following to your
>> build plugins config in maven:
>>
>> <plugin>
>> <groupId>org.apache.maven.plugins</groupId>
>> <artifactId>maven-checkstyle-plugin</artifactId>
>> <version>${cs.checkstyle.version}</version>
>> <dependencies>
>> <dependency>
>> <groupId>org.apache.cloudstack</groupId>
>> <artifactId>checkstyle</artifactId>
>> <version>0.0.1-SNAPSHOT</version>
>> </dependency>
>> </dependencies>
>> <executions>
>> <execution>
>> <phase>process-sources</phase>
>> <goals>
>> <goal>check</goal>
>> </goals>
>> </execution>
>> </executions>
>> <configuration>
>> <failsOnError>true</failsOnError>
>> <configLocation>tooling/checkstyle.xml</configLocation>
>> <consoleOutput>true</consoleOutput>
>> <includeTestSourceDirectory>true</includeTestSourceDirectory>
>> <sourceDirectory>${project.basedir}</sourceDirectory>
>>
>> <includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
>> <excludes>**\/target\/,**\/bin\/</excludes>
>> </configuration>
>> </plugin>
>>
>>
>> For now its voluntary, but i would like your opinion on making this a
>> mandatory part of the build process. Meaning a compile with not succeed
>> when check style reports errors.
>>
>> Cheers,
>>
>> Hugo
RE: checkstyle
Posted by Donal Lafferty <do...@citrix.com>.
How does it compare to previous versions for CloudStack? E.g. http://markmail.org/message/yz6qa2v47cdeic4d
> -----Original Message-----
> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> Sent: 04 November 2013 14:30
> To: dev@cloudstack.apache.org
> Subject: checkstyle
>
> Hey,
>
> Just added a very basic checkstyle configuration to maven. The configuration
> file is in parents/checkstyle and it checks just a few very basic things, like
> trailing whitespace and tabs where there should be spaces.
>
> I've enabled it for a single plugin to just the impact on build time and the
> amount of generated errors. Quite considerable, but i hope other parts of
> the code are better ;-)
>
> You can enable check style for your plugin by adding the following to your
> build plugins config in maven:
>
> <plugin>
> <groupId>org.apache.maven.plugins</groupId>
> <artifactId>maven-checkstyle-plugin</artifactId>
> <version>${cs.checkstyle.version}</version>
> <dependencies>
> <dependency>
> <groupId>org.apache.cloudstack</groupId>
> <artifactId>checkstyle</artifactId>
> <version>0.0.1-SNAPSHOT</version>
> </dependency>
> </dependencies>
> <executions>
> <execution>
> <phase>process-sources</phase>
> <goals>
> <goal>check</goal>
> </goals>
> </execution>
> </executions>
> <configuration>
> <failsOnError>true</failsOnError>
> <configLocation>tooling/checkstyle.xml</configLocation>
> <consoleOutput>true</consoleOutput>
> <includeTestSourceDirectory>true</includeTestSourceDirectory>
> <sourceDirectory>${project.basedir}</sourceDirectory>
>
> <includes>**\/*.java,**\/*.xml,**\/*.ini,**\/*.sh,**\/*.bat</includes>
> <excludes>**\/target\/,**\/bin\/</excludes>
> </configuration>
> </plugin>
>
>
> For now its voluntary, but i would like your opinion on making this a
> mandatory part of the build process. Meaning a compile with not succeed
> when check style reports errors.
>
> Cheers,
>
> Hugo