A new name and API for MavenImporter

[mjobanek]

Hi all,

after some time, I'm reopening the issue related to renaming of MavenImporter class. There where some discussions around that on Jira [1], GitHub [2] and on Andrew's Twitter [3]. However, there was no agreement on the name. Additionally, I think that subsequently the whole API should change as well and that hasn't been solved either.

First of all I'd like to ask you:

Does anyone use this feature (MavenImporter) or is interested in it? I want to find out whether the effort of renaming/refactoring and maybe some additional implementation would be worth it...

My proposals:

Having considered all the suggestions and approaches I'm proposing three options (the first one is my favorite :-)):

1)

 ShrinkWrap.create(ArchiveMavenAssembler.class, "archive.war")
        .configuredFromFile("/path/to/settings.xml")    
        .usingPomFile("/path/to/pom")            
        .withBuildOutput()
        .withTestBuildOutput()
        .withDependencies()                
        .as(WebArchive.class)    

2)

ShrinkWrap.create(ArchiveMavenAssembler.class)
        .configuredFromFile("/path/to/settings.xml")
        .usingPomFile("/path/to/pom")            
        .addBuildOutput()
        .addTestBuildOutput()
        .addDependencies()        
        .produce(WebArchive.class, "archive.war");    

3)

ShrinkWrap.create(ArchiveUsingMaven.class, "archive.war")
        .withMavenConfiguredFromFile("/path/to/settings.xml")
        .withPomLoadedFromFile("/path/to/pom")            
        .addBuildOutput()            
        .addTestBuildOutput()
        .addDependencies()        
        .as(WebArchive.class);        

Do you like it or do you hate it? Please tell me what you think...

I guess that EmbeddedGradleImporter should be renamed as well. Too keep the names consistent I would choose ArchiveGradleAssembler (ArchiveUsingGradle). The API could be the same as it is now except for methods "importBuildOutput" - they would be renamed analogically.

The current implementation (API) will be kept till next major release and marked as "@Depracated".

[1] https://issues.jboss.org/browse/SHRINKRES-132

[2] https://github.com/shrinkwrap/resolver/pull/49

[3] https://twitter.com/ALRubinger/status/360141924742606848

Thank you

Matous

[kpiwko]

I like option 2. the best Matous!

Karel

[lfryc]

Hey Matous,

I like the option 1 as well since it is consistent with other Archives.

I like ArchiveMavenAssembler

[mjobanek]

Thank you guys
What is your opinion aslak alrubinger mmatloka bmajsak tair.sabirgaliev

[tair.sabirgaliev]

Hi Matous,

I also like the 1st variant, except the place where the archive name is specified. The 2nd option puts archive name right after archive type, this is more convenient, IMO.

[mjobanek]

Thanks tair.sabirgaliev for your comment.

Yes, the place where the archive name is specified is not the best, but this is a functionality of the method ShrinkWrap.create(Class<T> type, String archiveName) and this cannot be changed. The idea that I have in the 2nd variant is that you can specify the name on both places (create() and produce()) - if you specified it on both places at the same time the produce() method would have a priority (it would overwrite the name specified in create()). Of course, this could be confusing so elaborated javadoc is necessary.

[bmajsak]

I think the first option is the most appealing for me, with two tweaks:

1) I would remove File suffixes - usingPom(String pathToPomFile) is enough from the API standpoint (the same applies to settings.xml)

2) I would move archive name ShrinkWrap.create(ArchiveMavenAssembler.class).....as(WebArchive.class, "archive.war")

For the second point - we don't give a name to ArchiveMavenAssembler, like in other cases when we use ShrinkWrap.create, so I would say it's better to "break the rule", rather than having confusing API.

[mjobanek]

Good points!

So, the resulting API would be:

ShrinkWrap.create(ArchiveMavenAssembler.class)
          .configuredFrom("/path/to/settings.xml")
          .usingPom("/path/to/pom")
          .withBuildOutput()
          .withTestBuildOutput(
          .withDependencies()
          .as(WebArchive.class, "archive.war");

Yeah, this seems to be more logic and clear.

Thanks!