maven-shade-plugin icon indicating copy to clipboard operation
maven-shade-plugin copied to clipboard

[MSHADE-307] adding useDefaultConfiguration flag in ShadeMojo

Open rmannibucau opened this issue 6 years ago • 15 comments

Following this checklist to help us incorporate your contribution quickly and easily:

  • [X] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • [X] Each commit in the pull request should have a meaningful subject line and body.
  • [X] Format the pull request title like [MSHADE-XXX] - Fixes bug in ApproximateQuantiles, where you replace MSHADE-XXX with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
  • [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [X] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • [X] You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.

rmannibucau avatar Dec 15 '18 20:12 rmannibucau

Common sounds a good alternative but too generic, I thought about "auto" but was not that happy too. Maybe "autoConfigure" + adding a spi on transformers and filters would make more sense?

rmannibucau avatar Dec 15 '18 23:12 rmannibucau

autoConfigure + SPI will be great, but it a more complex change.

The other way would be to have such "useDefaultConfiguration", turned on by default, but with a bump in the major version for the plugin.

@rfscholte what do you think ?

eolivelli avatar Dec 16 '18 07:12 eolivelli

In general I'd like prefer to keep the number of parameters to a minimum. So if we want to do this, why not set these as defaults. When there are explicit filters/transformers, these will be used. Any empty list would mean: no filters or transformers. And this implies there should be an explicit filter for signatures.

rfscholte avatar Dec 16 '18 10:12 rfscholte

Changing defaults needs a bump in major version ?

These new defaults are safe but not obvious.

I am +1 in changing defaults

eolivelli avatar Dec 16 '18 10:12 eolivelli

The new version will be 3.n+1.0. The first 3 means that is requires at least Maven 3. We discovered that this pattern makes is very clear when certain plugins can be used. We should add this change to the index.html file too, just to be clear. (in the end the result should be the same, because if filters and transformers are specified, nothing will change)

rfscholte avatar Dec 16 '18 10:12 rfscholte

Hmm, i thought of that - active by default and ignored if set by the user - but it is rare it is sufficient and does need a log4j, openwebbeans or other transformer so i preferred the composition. Should we have it with 3 values: default = use, skip and merge?

rmannibucau avatar Dec 16 '18 11:12 rmannibucau

Okay for versioning and index.html I can take of take (with a little guidance).

For use/skip/merge....things are getting complicated and complicated.

We should default to 'merge', but will 'use' be useful? As 'merge' is a super set of 'use'

eolivelli avatar Dec 16 '18 11:12 eolivelli

For use/skip/merge....things are getting complicated and complicated.

I totally agree here. Like all Maven plugin parameters it is: use the default or override it. Those are the only 2 modes, clear and simple.

rfscholte avatar Dec 16 '18 12:12 rfscholte

Hmm it must be combinable or it is useless and config stays complex.

Merge (default), override, skip?

rmannibucau avatar Dec 16 '18 12:12 rmannibucau

Sorry, but it will be default or override: easy to understand and maximum control. If you want full merge/override-control, you need to do that with the magic attributes as described at https://blog.sonatype.com/2011/01/maven-how-to-merging-plugin-configuration-in-complex-projects/

rfscholte avatar Dec 16 '18 12:12 rfscholte

@rfscholte this is my intention but here it does not work cause the config is implicit, this is why i proposed 3 states. Now if you say the impl is ok to turn on by default it can be enough for the next release (and we enhance later if needed). If so, do you have a name preference?

rmannibucau avatar Dec 16 '18 15:12 rmannibucau

If so, do you have a name preference?

for what?

rfscholte avatar Dec 16 '18 15:12 rfscholte

@rfscholte "useDefaultConfiguration" was not liked by @eolivelli and I have to admit I'm not fan of "common" usage so asking for options here

I will try to PoC the SPI idea now to let us "see it" before going with your other options which seems to kill the idea to simplify the config of the plugin.

rmannibucau avatar Dec 16 '18 15:12 rmannibucau

there's no need for this parameter. Just remove it

rfscholte avatar Dec 16 '18 15:12 rfscholte

Guys, I added the SPI and removed the toggle, can you have a look and let me know if it looks better this way? I like this solution cause we can plug the SPI in log4j, openwebbeans, and other extensions

rmannibucau avatar Dec 16 '18 15:12 rmannibucau