Fix #143 Enhance overrideFile with overrideUrl #181
Conversation
|
@Jurrie, could you please review? |
| { | ||
| try | ||
| { | ||
| return deprecatedFile.toURI().toURL(); |
There was a problem hiding this comment.
Should a warning be emitted telling the user he is using a deprecated setting?
There was a problem hiding this comment.
I did not want to clutter this method with yet another argument. Let me have a look if I can add the warning elsewhere.
There was a problem hiding this comment.
Can't you just do a getLogger().warn("!!! overrideFile is deprecated, use now overrideUrl !!!"); in the body of the if that's on line 95?
| /** | ||
| * Hidden | ||
| */ | ||
| private LicenseMojoUtils() |
There was a problem hiding this comment.
Shouldn't the class be declared final? After all, it's a utility class with a private constructor...
| * @since 1.17 | ||
| */ | ||
| @Parameter( property = "license.overrideUrl" ) | ||
| private String overrideUrl; |
There was a problem hiding this comment.
Same as PR 153: I think it should be documented what the file contents should look like.
There was a problem hiding this comment.
The code seems to expect a property file but looking at DefaultThirdPartyTool, I am not able to tell quickly enough what mapping should that be. Names to names or URLs to URLs or something else? Where is that mapping used, actually?
There was a problem hiding this comment.
I think it's documented here. I simply needed the ability to share an override file with other teams in my company, so I didn't investigate where in the code the mapping was used.
|
5bda7d7 fixes the formatting |
|
There is a hint about the override file content in license-maven-plugin/src/site/apt/examples/example-thirdparty.apt.vm Lines 356 to 360 in 5bda7d7 |
|
4587871 documents the override file format in the mojo params https://github.com/mojohaus/license-maven-plugin/pull/181/files#diff-0bfb2b7e70b9abacb4339555c7b49e81R268 and https://github.com/mojohaus/license-maven-plugin/pull/181/files#diff-9124975c1831b088259ed63a7d9de25bR202 @Jurrie could you please review and eventually give the final approval? |
Based on mojohaus#153 by @Jurrie with enhancements by @ppalaga
|
21d1a7b just a rebase on top of the current master |
@ppalaga all ok for me, but there's one thing left on emitting a warning when using deprecated stuff. Was it your intention to leave that as-is? |
|
Thanks for your contribution, @Jurrie ! |
Based on #153 by
@Jurrie with enhancements by @ppalaga