Maven plugin

[fzakaria]

Problem

This PullRequest is in response to Issue 229.
Essentially, I was going through the tutorial for finatra with Thrift and had made a new Java project ThriftModelExample with classifier idl that held only my thrift file.

My thrift file included "finatra-thrift/finatra_thrift_exceptions.thrift" from the finatra-thrift_2.11 artifact; however this artifact does not have the idl classifier.

Solution

The original solution was to include the following:

1. A fully configurable list for the Mojo of Modules (groupId & artifactId) that are forced included to be searched for thrift files.
2. For any Artifact that has the IDL classifier the plugin should automatically include all transitive dependencies of that artifact. This makes sense for all cases such as described in the problem statement above where you would want to decompose your Thrift files into many modules and make use of dependency resolution.
3. While I was at it, I also removed dead code that was never referenced
4. While I was at it, I migrated the plugin to use the maven plugin Annotations

Result

All dependencies of a project that is found to be either of classifier idl or forced inclusion has all of its dependencies included which allows for the proper modularization of thrift files.
It is also less strict about the classifier naming.

Con
The downside to this change is that I had to remove the test cases.
This new code makes use of the Aether maven API, however there does not seem to be a way to retrofit that into the maven plugin test harness.

[fzakaria]

I've uploaded - ThriftExplorer to demo the problem and solution.

You can see the pom.xml for the package that contains my idl

It expect the 4.6.0-SNAPSHOT so please do a maven install to install into your local repository

[mosesn]

these tests seem useful, why are you deleting them?

[mosesn]

Sorry it took us so long to review this! I don't know maven well, and we don't use it internally anymore, so our review will unfortunately have to be pretty cursory. Is there any chance you can replace the tests that don't work anymore with a new test that does something similar? LGTM otherwise.

[fzakaria]

@mosesn It's been a while since I worked on this codebase. I believe those tests were deleted because I couldn't find out how to transition them to the newer maven plugin model (annotation based) - I am also not terribly familiar with Maven plugin development but needed to fix the root issue for myself.

If we can keep the PR open a bit longer I can give it another stab on the weekend of reintroducing the tests.

[mosesn]

That would be great! No rush =)

[mosesn]

@fzakaria hey, have you had a chance to take a look at this again?

[mosesn]

@fzakaria I'm thinking maybe we should merge it without the tests? what do you think?

[fzakaria]

Let me check out the PR today and I'll go over it before noon PST.

[mosesn]

Rad, thanks!

[fzakaria]

@mosesn I remerged from develop and re-added the test class.
I had to comment out two of the tests cause I couldn't figure out how to make them work.
(I couldn't find any great documentation with incorporating aether with Maven plugin)

[codecov-io]

Current coverage is 27.25% (diff: 100%)

Merging #230 into develop will decrease coverage by 0.03%

@@            develop       #230   diff @@
==========================================
  Files            63         63          
  Lines          2792       2796     +4   
  Methods        2656       2659     +3   
  Messages          0          0          
  Branches        136        137     +1   
==========================================
  Hits            762        762          
- Misses         2030       2034     +4   
  Partials          0          0          

Powered by Codecov. Last update a96517c...3eb6b25

[cacoco]

@fzakaria your most recent changes seem to have a merge conflict (scrooge-maven-plugin/pom.xm), would you mind merging latest from the develop branch and trying to fix the conflicts. Also, I think we're going to want a way to have tests that are at least similar to the ones commented out.

[bryce-anderson]

@fzakaria, are you still working on this?

[fzakaria]

@bryce-anderson I'll close.
I do not have time to continue commiting to this branch.

Hopefully if someone hits same issue they can see the work done.
Thanks!