elharo commented on code in PR #23:
URL:
https://github.com/apache/maven-shared-incremental/pull/23#discussion_r1219477223
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -61,17 +69,28 @@
private MavenProject mavenProject;
/**
- * Used for detecting changes between the Mojo execution.
+ * Used for detecting changes in the output directory between the Mojo
execution.
Review Comment:
something's off with "between the Mojo execution". possibly should be
"between Mojo executions" or perhaps something else is meant?
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -323,25 +335,23 @@ public String[] beforeRebuildExecution(
IncrementalBuildHelperRequest incrementa
public void afterRebuildExecution( IncrementalBuildHelperRequest
incrementalBuildHelperRequest )
throws MojoExecutionException
{
- DirectoryScanner diffScanner = getDirectoryScanner();
- // now scan the same directory again and create a diff
- diffScanner.scan();
- DirectoryScanResult scanResult = diffScanner.diffIncludedFiles(
filesBeforeAction );
-
File mojoConfigBase = getMojoStatusDirectory();
- File mojoConfigFile = new File( mojoConfigBase,
CREATED_FILES_LST_FILENAME );
- try
- {
- FileUtils.fileWriteArray( mojoConfigFile,
scanResult.getFilesAdded() );
- }
- catch ( IOException e )
- {
- throw new MojoExecutionException( "Error while storing the mojo
status", e );
- }
+ writeChangedFiles(
+ getDirectoryScanner(),
Review Comment:
no need to call a getter method in the same class, just use the
directoryScanner field directly
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction )
+ {
+ // now scan the same directory again and create a diff
+ directoryScanner.scan();
+ DirectoryScanResult outputScanResult =
+ directoryScanner.diffIncludedFiles( filesBeforeAction );
+ return outputScanResult;
+ }
+
+ private static List<String> deleteFiles( File fileNameIndex, File parent )
throws IOException
+ {
+ List<String> oldFiles = Files.readAllLines( fileNameIndex.toPath() );
+ for ( String oldFileName : oldFiles )
+ {
+ File oldFile = new File( parent, oldFileName );
+ oldFile.delete();
Review Comment:
what if this method returns false? i.e. delete failed. Do we care?
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
Review Comment:
needs Javadoc
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -61,17 +69,28 @@
private MavenProject mavenProject;
/**
- * Used for detecting changes between the Mojo execution.
+ * Used for detecting changes in the output directory between the Mojo
execution.
* @see #getDirectoryScanner();
*/
- private DirectoryScanner directoryScanner;
+ private DirectoryScanner directoryScanner = new DirectoryScanner();
+
+ /**
+ * Used for detecting changes in the generated sources directory between
the Mojo execution.
Review Comment:
ditto
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
Review Comment:
This is a field in the same class so no need to pass it as an argument
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
Review Comment:
This is a constant in the same class so no need to pass it as an argument
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
Review Comment:
make non-static and use the directoryScanner field
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
Review Comment:
Just use String.join or StringJoiner here
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction )
+ {
+ // now scan the same directory again and create a diff
+ directoryScanner.scan();
+ DirectoryScanResult outputScanResult =
+ directoryScanner.diffIncludedFiles( filesBeforeAction );
+ return outputScanResult;
+ }
+
+ private static List<String> deleteFiles( File fileNameIndex, File parent )
throws IOException
+ {
+ List<String> oldFiles = Files.readAllLines( fileNameIndex.toPath() );
+ for ( String oldFileName : oldFiles )
+ {
+ File oldFile = new File( parent, oldFileName );
+ oldFile.delete();
+ }
+ return oldFiles;
+ }
+
+ private String[] scanDirectory( DirectoryScanner directoryScanner, File
directory )
Review Comment:
make non-static and use the directoryScanner field
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
Review Comment:
This is a constant in the same class so no need to pass it as an argument
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
Review Comment:
Is it worth checking here that the file exists and is a directory? Throw an
IllegalArgumentException if not.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
+ }
+
+ public IncrementalBuildHelperRequest generatedSourcesDirectory( File
generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
Review Comment:
Is it worth checking here that the file exists and is a directory?
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
Review Comment:
needs Javadoc that clearly explains what this directory is.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -61,17 +69,28 @@
private MavenProject mavenProject;
/**
- * Used for detecting changes between the Mojo execution.
+ * Used for detecting changes in the output directory between the Mojo
execution.
* @see #getDirectoryScanner();
*/
- private DirectoryScanner directoryScanner;
+ private DirectoryScanner directoryScanner = new DirectoryScanner();
Review Comment:
This class is deprecated. Consider using use java.nio.file.DirectoryStream
or java.nio.Files.walkFileTree() and related classes instead. Those should
definitely be safer in a multi-threaded environment.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction )
+ {
+ // now scan the same directory again and create a diff
+ directoryScanner.scan();
+ DirectoryScanResult outputScanResult =
+ directoryScanner.diffIncludedFiles( filesBeforeAction );
+ return outputScanResult;
+ }
+
+ private static List<String> deleteFiles( File fileNameIndex, File parent )
throws IOException
+ {
+ List<String> oldFiles = Files.readAllLines( fileNameIndex.toPath() );
+ for ( String oldFileName : oldFiles )
+ {
+ File oldFile = new File( parent, oldFileName );
+ oldFile.delete();
+ }
+ return oldFiles;
+ }
+
+ private String[] scanDirectory( DirectoryScanner directoryScanner, File
directory )
+ {
+ directoryScanner.setBasedir( directory );
Review Comment:
This is a little worrisome. I didn't realize directoryScanner was mutable.
This might suggest not using a directoryScanner field at all and just creating
one when we need it.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
+ }
+
+ public IncrementalBuildHelperRequest generatedSourcesDirectory( File
generatedSourcesDirectory )
Review Comment:
needs Javadoc that clearly explains what this directory is.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -66,12 +71,24 @@
*/
private DirectoryScanner directoryScanner;
+ /**
+ * Used for detecting changes between the Mojo execution.
+ * @see #getGeneratedSourcesDirectoryScanner() ;
+ */
+ private DirectoryScanner generatedSourcesDirectoryScanner;
+
/**
* Once the {@link
#beforeRebuildExecution(org.apache.maven.shared.incremental.IncrementalBuildHelperRequest)}
got
* called, this will contain the list of files in the build directory.
*/
private String[] filesBeforeAction = new String[0];
+ /**
+ * Once the {@link
#beforeRebuildExecution(org.apache.maven.shared.incremental.IncrementalBuildHelperRequest)}
got
Review Comment:
ping
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
