Minor cleanups after merging WAR Gradle support contribution #1072
Conversation
| @Nullable private Path explodedWarDirectory; | ||
|
|
||
| public void setWarFile(Path warFile) { | ||
| from(getProject().zipTree(warFile.toFile())); |
There was a problem hiding this comment.
I believe this can just be zipTree(warFile)
| into(explodedWarDirectory); | ||
| } | ||
|
|
||
| @OutputDirectory | ||
| @Nullable | ||
| public File getExplodedWarDirectory() { | ||
| public Path getExplodedWarDirectory() { |
There was a problem hiding this comment.
Hmm, I recall from before that Path does not work with @OutputDirectory up-to-date checks. We should confirm that trying to run this task again skips the task correctly.
There was a problem hiding this comment.
Huh, that's a good thing to know. Let me check, but I see we may need to revert it.
There was a problem hiding this comment.
Yeah, confirmed we should keep File for Gradle tasks.
| @@ -185,7 +179,7 @@ private static JavaLayerConfigurations getForWarProject( | |||
|
|
|||
| Path explodedWarPath = GradleProjectProperties.getExplodedWarDirectory(project); | |||
|
|
|||
| Path libOutputDirectory = explodedWarPath.resolve(WEB_INF_LIB); | |||
| Path libOutputDirectory = explodedWarPath.resolve("WEB-INF/lib"); | |||
There was a problem hiding this comment.
What was the reason for removing the constants here?
There was a problem hiding this comment.
There are pros and cons, but I thought pros of removing constants have more benefits, particularly for readability.
WEB_INF_LIBwill always be "WEB-INF/lib". If it's something we may change in the future (like some default value of our choice), I can see a good reason for having the constant declared in the top of the file, but in this case, it's always static.- Readability: I keep looking into the actual value of
WEB_INB_LIBto see whatlibOutputDirectoryactually is. With "WEB-INF/lib", it's directly recognizable. - Looking at the field
WEB_INF_LIBmakes me wonder at which places this constant is being referenced to.
There was a problem hiding this comment.
What do you think? I'm actually fine with other ways.
|
|
||
| } else if (Files.isDirectory(path)) { | ||
| try (Stream<Path> dirStream = Files.list(path)) { | ||
| if (!dirStream.findAny().isPresent()) { | ||
| try (Stream<Path> stream = Files.list(path)) { |
There was a problem hiding this comment.
nit: directoryStream/filesStream/files?
| .walk( | ||
| path -> { | ||
| AbsoluteUnixPath pathInContainer = | ||
| classesExtractionPath.resolve(webInfClasses.relativize(path)); | ||
|
|
||
| if (FileSystems.getDefault().getPathMatcher("glob:**.class").matches(path)) { |
There was a problem hiding this comment.
Oh I believe this can be simplified to just like path.getFilename().toString().endsWith(".class")
Minor cleanups after #923.