Skip to content

HSEARCH 1178 Add the checkstyle plugin #352

Closed
wants to merge 2 commits into from

3 participants

@DavideD
Hibernate member
DavideD commented Nov 11, 2012

https://hibernate.onjira.com/browse/HSEARCH-1178

Hi,
my experiment with the checkstyle plugin. I use a regular expression to check that the java classes copyright header match one of the existing copyrights. The expression is not very user friendly and it would probably make sense to create a custom rule for this but it seems to work. Adding a new copyright is quite easy it just requires to escape some characters and put it in one line.

Some classes didn't have a copyright so I've added it.

The checkstyle plugin will block the build if something is wrong, this seems reasonable at the moment because the only rule applied is the header check.

@hferentschik hferentschik was assigned Nov 11, 2012
@hferentschik

Why a different module for the build config. That seems like an overkill for me. IMO the checkstyle config should be in the parent pom. See also the prototype I made a while back - https://github.com/hferentschik/hibernate-search/commits/HSEARCH-1178

@hferentschik

I would have favored the Header module instead of the fiddly regexp. With the header module you can just point out a file which contains the header. Unfortunately, it only allows for a single file and hence fails, because we have basically two version of the license header. I discussed this once with Sanne, I would prefer aligning the code and use a single header.

@hferentschik
Hibernate member

my experiment with the checkstyle plugin.

and here is mine from a while back https://github.com/hferentschik/hibernate-search/commits/HSEARCH-1178
I guess I am not a big fan of the separate module. Which benefit do you see in using it?

I use a regular expression to check that the java classes copyright header match one of the existing copyrights. The expression is not very user friendly and it would probably make sense to create a custom rule for this but it seems to work. Adding a new copyright is quite easy it just requires to escape some characters and put it in one line.

Seems to me very fiddly. The Header module seems more appropriate, but it does not allow multiple formats. I guess an additional build-config module would make sense when we would create custom checkstyle rules. But should they really live with the Search code base?

Some classes didn't have a copyright so I've added it.

+1

The checkstyle plugin will block the build if something is wrong, this seems reasonable at the moment because the
only rule applied is the header check.

+1 That's the way I would like to set it up as well. Also I would like to add as many checks as possible which don't require much (or even no) work for now. Unfortunately, I don't think we have an agreement yet amongst the team that this is the best way to integrate checkstyle.

@DavideD
Hibernate member
DavideD commented Nov 12, 2012

I guess I am not a big fan of the separate module. Which benefit do you see in using it?

I've used a different module mainly because is the suggested way in the maven documentation (http://maven.apache.org/plugins/maven-checkstyle-plugin/examples/multi-module-config.html) and because the same approach is used in Shrinkwrap.

When I've seen this in the beginning I had the same reaction but after all this is just a project used during the build and it doesn't require any release (I think I have to disable the deploy in the pom configuration). It can also be used to store additional configuration files like the eclipse xml template and formatting rules (same approach used in Shrinkwrap). Anyway, I don't have a strong opinion on this and the reason I prefer it a bit is because in general I don't like to use properties in Maven if I can avoid it.

The Header module seems more appropriate...

+1
If it's possible to change all the copyrights I would go with it (do you think it's going to take long to decide?). I was actually thinking to take a look at the header plugin and see if it was possible to change it to support many headers (not enough time to check it though).

My idea would be to use the regex solution for the moment so that we can integrate the checkstyle plugin in the build and possibly start to add other rules. When the header will fit our scenario we can change the configuration file (it doesn't look complex to do ).

By the way, I'm sorry I picked up this issue while it was assigned to you, talking with @Sanne I thought no-one was working on it at the moment and I was curious to play with the plugin regular expressions.

@hferentschik
Hibernate member

I've used a different module mainly because is the suggested way in the maven documentation (http://maven.apache.org/plugins/maven-checkstyle-plugin/examples/multi-module-config.html) and because the same approach is used in Shrinkwrap.

Not sure if this is the best way. For me that's what the parent module is for. After all there is nothing else there. Adding shared configuration to the parent gives the whole module more weight imo.

If we go down this route we definitely would have to exclude it from deploy.

Another reason I am not a big fan of this is that is adds yet another module in the IDE. All these modules create confusion imo.

If it's possible to change all the copyrights I would go with it (do you think it's going to take long to decide?).

I think we came to the conclusion that it is ok, but we should probably sync once more with @emmanuelbernard and @Sanne.

By the way, I'm sorry I picked up this issue while it was assigned to you, talking with @Sanne I thought no-one was working on it at the moment and I was curious to play with the plugin regular expressions.

I was no actively working on this. I did the prototype quite some time back. It was sitting there, because there were two outstanding questions:

  • Can we align the license headers?
  • Do we agree how we want to integrate the plugin?

The latter was the biggest issue. As you I want to enable it per default and fail on violations.

@Sanne
Hibernate member
Sanne commented Nov 12, 2012

I think it would be easy to change the rules at a second time to move this from using the regex to the headers plugin; I would prefer merging this without waiting for the headers plugin so that we don't block integration of checkstyle while waiting for clarifications on the license & legal.

It would be great to polish then our checkstyle integration iteratively, for example as we already discussed to start fixing code style only after the 4.2 final release.

Do we agree how we want to integrate the plugin?

I think we should have Jenkins check it on pull requests as soon as we have that system. For now I think we can start experimenting with a "fail the build" approach on each build because the rules are very relaxed.

This pull addressed the HSEARCH-1178 description properly so I'm happy with it.

Can we align the license headers?

Not happy with that, and I see no rush in it since with the regex trick we can move forward.

@hferentschik
Hibernate member

Not happy with that, and I see no rush in it since with the regex trick we can move forward.

IMO the regexp approach is weak and I still think aligning the headers is the way to go, but to move forward it might be best to go ahead with this approach. I still vote -1 though for the additional module.

@Sanne
Hibernate member
Sanne commented Nov 12, 2012

IMO the regexp approach is weak and I still think aligning the headers is the way to go, but to move forward it might be best to go ahead with this approach.

Agreed it might be weak, but an error will be pointed out and we don't need to maintain it for long. So let's move forward with it, ok?

I still vote -1 though for the additional module.

I'm neutral on that. We might need it separate as soon as we'll have a module with a different license, which we've just been talking about in the clustering meeting (i.e. we'll talk about it soon - for now it's just relevant to say that we might need that).
I'm not the Maven expert here. I usually ask @hferentschik for an opinion so Hardy I think you should have the last word on it, but why not a new module? and why do other projects do the same?

Benefit of a module - out o Search - could be that we can share it across all Hibernate projects. We could also specify it for each of our modules individually so that it doesn't interfere with temporary (Solr analyzers?) or permanent new modules.

@hferentschik
Hibernate member

We might need it separate as soon as we'll have a module with a different license, which we've just been talking about in the clustering meeting

why would that require a module? And how can you have a module with a different license in Search and still share the same parent? That seems dubious at best. However, that is a different discussion.

but why not a new module?

Let me ask you, why a module? ;-) Give me a compelling reason to put this into its own module? As of why not - more code & more confusion in the IDE

and why do other projects do the same?

not sure. I guess the idea being that you have a common place to collect things like checkstyle, IDE settings etc, but I fail to see why I could not just have a build_config under src of the parent/aggregator pom?

@Sanne
Hibernate member
Sanne commented Nov 12, 2012

why would that require a module? And how can you have a module with a different license in Search and still share the same parent? That seems dubious at best. However, that is a different discussion.

Someone once forked the Solr sourcecode as we needed a release for our analyzers and dumped it in our repo. That was dubious indeed but practical and I'm actually happy you did it.
I agree this is not the best place to discuss it so I won't provide more details here but it's relevant to the checkstyle integration to stress that we soon might need a module with different licensing terms. (soon=Lucene4 timeframe)

As of why not - more code & more confusion in the IDE

Doesn't create more confusion at all IMHO, actually you wouldn't even need to import such a module in the IDE for day to day work (any activity except refining the checkstyle rules). If that's confusing in IDEA, I don't know; what I know is that when I say that I can't work on ORM because Eclipse can't import it I have to hear that I should use a different IDE.

Let me ask you, why a module? ;-) Give me a compelling reason to put this into its own module?

  • Because composition wins over inheritance
  • Because you can include external resources rather than having to encode it all in a single XML file (and pack it up in a jar)
  • Because you can apply it selectively on some modules

But I'm just playing devil's advocate to have your thoughts on these points.. as said above I think you're the most skilled here with build tools and I'm happy to back any decision you make. The only output I'll dislike is the lack of a quick decision as either way this would be a failure to make progress ;-)

@hferentschik
Hibernate member

Because composition wins over inheritance

I fail to see these two principals at play here. Mind you, even in the build module approach the checkstyle plugin itself is configured in the parent pom (you only have the config file in a separate module).

Because you can include external resources rather than having to encode it all in a single XML file (and pack it up in a jar)

We can as many resources as we like to the parent pom. The only advantage I see is when we would build out own checkstyle module and need to compile checkstyle specific code. In this case, however, this module would become probably a checkstyle specific module not so much suited anymore for hosting other type of build configuration files. Also in this scenario you probably would be better off creating a standalone project for your checkstyle extension in order to encourage other projects to use it as well.

Because you can apply it selectively on some modules

There is no difference in the approach. As said, even in this approach the configuration of the plugin is done in the pluginManagement section of the parent pom and modules which want to use the plugin need to add it to their plugin list. No difference there.

But I'm just playing devil's advocate to have your thoughts on these points..

Sure.

@Sanne
Hibernate member
Sanne commented Nov 13, 2012

There is no difference in the approach. As said, even in this approach the configuration of the plugin is done in the pluginManagement section of the parent pom and modules which want to use the plugin need to add it to their plugin list. No difference there.

Right, we can the move it out in future if we need to support multiple different configurations.

@hferentschik
Hibernate member

Right, we can the move it out in future if we need to support multiple different configurations.

+1

@hferentschik
Hibernate member

I created a new pull request #354 which is based on Davide's work and put some of the things from my prototype in (most notably there is no dedicated build-config module)

@hferentschik
Hibernate member

Closing this pull request. Let's discuss #354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.