-
Notifications
You must be signed in to change notification settings - Fork 155
Add separate plugin parameter to filter dependencies #400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add separate plugin parameter to filter dependencies #400
Conversation
Enclosing `if` statement is not needed because in case there are no filter names `createFilterString` will create an empty string.
To ensure that no API newer than the target version is used by accident, when building with a newer JDK.
if (inFilter != null) { | ||
dependencyInjarFilterList.add(inFilter); | ||
if (inDependenciesFilter != null) { | ||
dependencyInjarFilterList.add(inDependenciesFilter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative to adding a new parameter could be to reuse the existing inLibsFilter
for this case here as well. That might be a bit more appropriate than the previous inFilter
.
But maybe adding a new parameter (as currently done in this pull request) is still the cleanest approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backwards compatibility this could use inFilter if inDependenciesFilter is null but inFilter non-null. But maybe it is better to not do too much complexity in name of compatibility. This is an easy change for users if they use the filters.
<source>1.8</source> | ||
<target>1.8</target> | ||
<release>8</release> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: This will require JDK >= 9 for building because it uses javac --release
, but I guess it is safe to assume that most users will have that.
(This is not technically needed for the other changes in this pull request.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring JDK 9 is fine. We could even change this to 11. I have no intention to maintain this for older versions.
See #393 (comment)