[MASSEMBLY-617] : Add new FileMapper for giving a suffix to filename … #14

tcollignon · 2018-07-14T22:58:17Z

…-> SuffixFileMapper

HI, refer to #13 , @plamentotev tell me to use FileMapper instead of adding just a fileNameSuffix. So this PR intend to add a new FileMapper, who can add a suffix to filename.

…-> SuffixFileMapper

plamentotev

Hi,

Thank you. I have a couple of comments:

I'm not sure what is the use case when you'll want the suffix to be added on the directory name. For example why I'll want csuffix.c\\xyz.gif instead of c.c\\xyzsuffix.gif
Please update the documentation as well - plexus-io/src/site/apt/filemappers.apt

plamentotev · 2018-07-17T04:07:08Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+        {
+            if ( name.contains( "." ) )
+            {
+                String beforeExtension = name.substring( 0, name.indexOf('.') );


Missing white space around '.'.

plamentotev · 2018-07-17T04:20:41Z

src/test/java/org/codehaus/plexus/components/io/filemappers/FileMapperTest.java

+        testFileMapper( new SuffixFileMapper(), SAMPLES, results );
+        testFileMapper( (SuffixFileMapper) lookup( FileMapper.ROLE, SuffixFileMapper.ROLE_HINT ), SAMPLES, results );
+
+        results = new String[] {null, null, "asuffix", "xyzsuffix.gif", "b/asuffix", "b/xyzsuffix.gif", "b\\asuffix", "b\\xyzsuffix.gif", "csuffix.c/a", "csuffix.c/xyz.gif", "csuffix.c\\a", "csuffix.c\\xyz.gif"};


Splinting on two lines would make it more readable and there is missing space after the curly brace.

tcollignon · 2018-07-21T21:15:00Z

I @plamentotev,

You're right, I have not managing directory in my Mapper, I have do it in this new commit. I have add documentation like you said, and add all of example in it to describe each case.
I hope it's good, if not tell me.

plamentotev

In general looks good. Just few more comments. I've missed some of the things the last time - sorry about that.

plamentotev · 2018-07-23T17:24:22Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+    public static String getMappedFileName( String suffix, String name )
+    {
+        String nameWithSuffix = name;
+        if ( StringUtils.isNotBlank( suffix ) )


The rest of the mappers throw IllegalStateException if any of the params is not initialized. Would be nice if this mapper is consistent with the others (PrefixFileMapper is exception but I think it would be better if it stays the only exception).

plamentotev · 2018-07-23T17:28:25Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+    /**
+     * Performs the mapping of a file name by adding a suffix.
+     */
+    public static String getMappedFileName( String suffix, String name )


I guess you followed the PrefixFileMapper code, but the rest does not have public static String getMappedFileName and I think it would be best to be consistent with them as PrefixFileMapper is a kind of exception. If you don't have other concerns I think it's best to move it to public String getMappedFileName( @Nonnull String name ).

Ok I have no problem with that
Indeed like you said I'm basically inspired of PrefixFileMapper because I didn't know plexus-io :(

plamentotev · 2018-07-23T17:30:03Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+        {
+            final int dirSep = Math.max( name.lastIndexOf( '/' ), name.lastIndexOf( '\\' ) );
+            String filename = dirSep > 0 ? name.substring( dirSep +1 ) : name;
+            String dirname = dirSep > 0 ? name.substring( 0, dirSep +1 ) : "";


I hope I don't get annoying with that but I think there is missing white space after the plus sign.

No problem code style must be respected

plamentotev · 2018-07-23T17:48:23Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+        String nameWithSuffix = name;
+        if ( StringUtils.isNotBlank( suffix ) )
+        {
+            final int dirSep = Math.max( name.lastIndexOf( '/' ), name.lastIndexOf( '\\' ) );


I don't think you need to split the file name into file name and directory name. See FileExtensionMapper for example. But feel free to leave it like that if you think it is more readable this way.

Yes actually I try to do the same thing as FileExtensionMapper but I want to handle the case of multiple "." in extension (like tar.gz). So I have added a new test case to check that is correctly handle by the SuffixMapper. So I it ok for you I give my code who works with this case (FileExtensionMapper does not handle that case)

plamentotev · 2018-07-23T17:55:26Z

src/site/apt/filemappers.apt

+* {Suffix File Mapper}
+
+  The {{{./apidocs/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.html}suffix
+  file mapper}} add the given suffix to the filename. The suffix will be added before the file


I think the correct is adds the given

oops you see my bad English :(

plamentotev · 2018-07-23T17:58:17Z

src/site/apt/filemappers.apt

+  The {{{./apidocs/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.html}suffix
+  file mapper}} add the given suffix to the filename. The suffix will be added before the file
+  extension. Examples : 
+  theFile.txt => theFileNiceSuffix.txt


The examples will be displayed on one line as the new lines will be stripped. If you wrap them as code that would solve the problem and they would stand out as well. And would be great if you add example with dot in the file name as it would make clear how the mapper behaves in such cases.

I have still the following example : dir/archive.tar.gz => dir/archiveNiceSuffix.tar.gz
this is not enough ? If not please tell me an example

It is ok. My bad, didn't read the docs carefully enough. Sorry about that.

plamentotev · 2018-07-23T18:02:46Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+import org.codehaus.plexus.util.StringUtils;
+
+/**
+ * A file mapper, which maps by adding a suffix.


Would be nice if make it clear that the suffix is added before the dot. It is mentioned in the site docs but it would be nice if you state it here as well (so it is visible in the IDEs).

plamentotev · 2018-07-30T16:45:42Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

@@ -0,0 +1,74 @@
+package org.codehaus.plexus.components.io.filemappers;
+
+import java.util.regex.Matcher;


I think that is unused import.

plamentotev · 2018-07-30T16:50:52Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+import java.util.regex.Matcher;
+
+/*
+ * Copyright 2007 The Codehaus Foundation.


I've just noticed that this is the old license header. Judging by the other new files in the codehaus-plexus project you can just delete this line and keep the rest (the Apache license).

plamentotev · 2018-07-30T17:02:49Z

src/main/java/org/codehaus/plexus/components/io/filemappers/SuffixFileMapper.java

+     */
+    public void setSuffix( String suffix )
+    {
+        this.suffix = suffix;


As null is not valid value you can add check if suffix is null:

if ( suffix == null ) { throw new IllegalArgumentException( "The suffix is null." ); }

Not all marchers all have that (the regex one does not have) but I think as most have it is better this way.

tcollignon · 2018-07-31T04:32:47Z

Ok I have fix all of this @plamentotev

plamentotev · 2018-07-31T18:04:01Z

Thank you for your contribution. I know there were a lot of comments but as you can see the different mappers have slightly different behavior and that can be confusing. thank you for having the patience to make your mapper consistent with the majority of the other mappers :)

I've bumped the Plexus IO version, made some small formatting changes, squashed the commits and merged them in master. Also I've striped some extra white spaces at the end of empty lines.

[MASSEMBLY-617] : Add new FileMapper for giving a suffix to filename …

9370b32

…-> SuffixFileMapper

plamentotev added the enhancement label Jul 17, 2018

plamentotev mentioned this pull request Jul 17, 2018

[MASSEMBLY-617] add ability to give a fileSuffix to a AbstractPlexusI… #13

Closed

plamentotev requested changes Jul 17, 2018

View reviewed changes

Take care of directory, and add documentation

94833d1

fixing code style

b29e094

plamentotev reviewed Jul 23, 2018

View reviewed changes

Add new test case, code style, improve doc, remove static method

9ffe46a

plamentotev reviewed Jul 30, 2018

View reviewed changes

Import unused, Licence, add null check in setter

8954c8e

plamentotev added this to the 3.1.0 milestone Jul 31, 2018

plamentotev approved these changes Jul 31, 2018

View reviewed changes

plamentotev closed this in 6a843bd Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MASSEMBLY-617] : Add new FileMapper for giving a suffix to filename … #14

[MASSEMBLY-617] : Add new FileMapper for giving a suffix to filename … #14

tcollignon commented Jul 14, 2018

plamentotev left a comment

plamentotev Jul 17, 2018

plamentotev Jul 17, 2018

tcollignon commented Jul 21, 2018

plamentotev left a comment

plamentotev Jul 23, 2018

plamentotev Jul 23, 2018

tcollignon Jul 25, 2018

plamentotev Jul 23, 2018

tcollignon Jul 25, 2018

plamentotev Jul 23, 2018

tcollignon Jul 25, 2018

plamentotev Jul 23, 2018

tcollignon Jul 25, 2018

plamentotev Jul 23, 2018

tcollignon Jul 25, 2018

plamentotev Jul 30, 2018

plamentotev Jul 23, 2018

plamentotev Jul 30, 2018

plamentotev Jul 30, 2018

plamentotev Jul 30, 2018

tcollignon commented Jul 31, 2018

plamentotev commented Jul 31, 2018

		@@ -0,0 +1,74 @@
		package org.codehaus.plexus.components.io.filemappers;

		import java.util.regex.Matcher;

[MASSEMBLY-617] : Add new FileMapper for giving a suffix to filename … #14

[MASSEMBLY-617] : Add new FileMapper for giving a suffix to filename … #14

Conversation

tcollignon commented Jul 14, 2018

plamentotev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcollignon commented Jul 21, 2018

plamentotev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcollignon commented Jul 31, 2018

plamentotev commented Jul 31, 2018