[MASSEMBLY-886] Add fileMappers to BaseFileSet#92
[MASSEMBLY-886] Add fileMappers to BaseFileSet#92tcollignon wants to merge 2 commits intocodehaus-plexus:masterfrom
Conversation
There was a problem hiding this comment.
Hi,
It would be great if you add a test that verifies that your change works and make sure that future changes won't break it. While there is no suitable existing tests in AbstractArchiverTest, I think ZipArchiverTest#testAddArchivedFileSet would do the trick. Among other things it verifies that the InputStreamTransformer is working. Maybe you can add FileMapper and verify it is working. This is just a suggestion of course - feel free to add the test where you think it is most suitable.
Otherwise it looks good to me.
| InputStreamTransformer getStreamTransformer(); | ||
|
|
||
| /** | ||
| * Returns a set of file mappers, which should be used |
There was a problem hiding this comment.
I think it would be better if the comment explicitly states that only the file names are changed. Otherwise combined with the bit misleading name of FileMapper somebody may get the false impression that the files themselves are changed. Of course a quick look at the FileMapper docs would make that clear but I think it will be more developer friendly if the comment is explicit.
There was a problem hiding this comment.
of course you right, I add more explicit comment
| * Sets a set of file mappers, which should be used | ||
| * to change included files. | ||
| */ | ||
| public void setFileMappers( FileMapper[] fileMappers ) |
…mentation in javadoc
|
HI You've right I need to add a test ! Thx for you help ! |
|
Thank you for your contribution. I've bumped the Plexus Archiver version (new API is introduced) , squashed the commits and merged them in |
2 participants
Hi
Refer to #91, this PR intend to add an Array of FileMapper to BaseFileSet.
This will fix [MASSEMBLY-886] and [MASSEMBLY-617]