maven-shade-plugin
maven-shade-plugin copied to clipboard
[MSHADE-307] adding useDefaultConfiguration flag in ShadeMojo
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 replaceMSHADE-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.
-
[ ] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
-
[ ] In any other case, please file an Apache Individual Contributor License Agreement.
-
[X] I'm already an ASF committer
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?
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 ?
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.
Changing defaults needs a bump in major version ?
These new defaults are safe but not obvious.
I am +1 in changing defaults
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)
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?
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'
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.
Hmm it must be combinable or it is useless and config stays complex.
Merge (default), override, skip?
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 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?
If so, do you have a name preference?
for what?
@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.
there's no need for this parameter. Just remove it
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