desruisseaux commented on code in PR #362:
URL:
https://github.com/apache/maven-clean-plugin/pull/362#discussion_r4122624234
##########
src/main/java/org/apache/maven/plugins/clean/CleanMojo.java:
##########
@@ -149,10 +149,10 @@ public class CleanMojo implements
org.apache.maven.api.plugin.Mojo {
/**
* Indicates whether the build will continue even if there are clean
errors.
*
- * <p><b>Note:</b> when {@link #fast} is {@code true}, this parameter has
no effect on the
- * background deletion path. A session-end listener cannot structurally
fail the build in
- * Maven, so errors from background deletions are always logged as
warnings regardless of
- * this setting. Use {@code fast=false} if you need the build to fail on
clean errors.</p>
+ * <p>When {@link #fast} is {@code true} (the default) and this parameter
is {@code true} (the default),
+ * the deletion of each directory runs synchronously so that any error can
fail the build
Review Comment:
Il the deletion is run synchronously, then what is the purpose of moving it
to a temporary directory before to delete it?
##########
src/main/java/org/apache/maven/plugins/clean/CleanMojo.java:
##########
@@ -276,7 +275,15 @@ public void execute() {
if (fast && session != null) {
Path tmpDir = fastDir;
if (tmpDir == null) {
- tmpDir =
session.getRootDirectory().resolve("target").resolve(".clean");
+ Path rootDir;
+ try {
+ rootDir = session.getRootDirectory();
+ } catch (IllegalStateException e) {
+ rootDir = null;
+ }
+ tmpDir = rootDir != null
+ ? rootDir.resolve("target").resolve(".clean")
+ :
Path.of(System.getProperty("java.io.tmpdir")).resolve(".clean");
Review Comment:
This code has the following problems:
* It still have the `.` prefix in the `clean` directory name.
* It is more convolved than necessary with its translation of
`IllegalStateException` into `null` followed by a null-check.
* In case of failure, it creates a directory names `.clean` (hard-coded)
with no guarantee that this directory does not already exists.
Consider the following instead:
```java
try {
tmpDir = session.getRootDirectory().resolve("target").resolve("clean");
} catch (IllegalStateException e) {
log.debug("Missing root directory.", e);
tmpDir = Files.createTempDirectory("maven-clean-");
}
```
--
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]
