convert ConcatFilesTask.groovy to .java #34476
Conversation
Create ConcatFilesTask.java
|
Thanks for the PR @mara2701. It looks like this is incomplete? The .groovy file still exists, and the java implementation doesn't have a TaskAction annotated method, nor input/outputs. |
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Thanks for working on this @mara2701 ! We appreciate your effort.
I think this is a good starting point, I added some comments.
I also added more details to #34459 as to what we expect with this transition,
sorry I should have added these to begin with.
| public void concatFiles() { | ||
| final StringBuilder output = new StringBuilder(); | ||
|
|
||
| if (headerLine != null) { |
There was a problem hiding this comment.
This check is not necessary, Gradle doesn't allow for it unless the task is marked @Optional
|
|
||
| final StringBuilder sb = new StringBuilder(); | ||
| for (File f: files) { | ||
| Scanner scanner = new Scanner(f, "UTF-8" ); |
There was a problem hiding this comment.
We should use a try-with-resources syntax with this.
| final StringBuilder sb = new StringBuilder(); | ||
| for (File f: files) { | ||
| Scanner scanner = new Scanner(f, "UTF-8" ); | ||
| String text = scanner.useDelimiter("\\A").next(); |
There was a problem hiding this comment.
I think using nextLine might be easier to read, and since we need all the lines in memory to perform the de-duplication anyhow, I would consider doing it in one step.
Using java streams and a set as the Groovy implementation did to take care of the de-duplication would make this concise and easy to understand:
files.getFiles().stream()
.flatMap(Files.readLines(null, StandardCharsets.UTF_8).stream())
.collect(Collectors.toSet())
.forEach( line -> {
// write each line to target
})
You will probably have to collect to a different set implementation to preserve order but this is the general idea. Writing a test will make that obvious.
|
@mara2701 Are you planning to work further on this? |
|
Picking this task due to long inactivity. |
|
This PR might be obsolete due to #37497. |
|
Closing since #37497 was merged. |
No description provided.