-
Notifications
You must be signed in to change notification settings - Fork 25.6k
refactor: convert FileContentsTask to Java #34539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: convert FileContentsTask to Java #34539
Conversation
|
I was not sure about test naming conventions. |
|
Pinging @elastic/es-core-infra |
alpar-t
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @baptistemesta !
This looks good, just left a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer adding the set prefix.
| public void contents(Object contents) { | |
| public void setContents(Object contents) { |
Also in this particular case I think we should also make a semantic change and type contents with String. This does mean that groovy will resolve GStrings sooner, but looking at the uses of this task that is ok and I would prefer the stronger typing, it's one of the reasons we convert in the first place.
We can always lather add a version that takes Supplier<String> if we need it to be more dynamic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public void file(Object file) { | |
| public void setFile(File file) { | |
| this.file = file; | |
| } | |
| public void setFile(String file) { | |
| this.file = getProject().file(file); | |
| } |
Along the same lines as above to avoid the use of Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we use something like Files.write where we can be specific about the encoding.
FileWriter will rely on the default encoding, something we generally try to avoid.
|
@elasticmachine test this please |
|
I integrated changes.
I have an issue with the Files.writeString, build fails with: |
The method is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equals is ok, other parts of the build use it as well and I think it's the direction Gradle is going into w.r.t. task properties.
We could do without the as File if we have a setFile(String file) method, see my previous suggestion.
|
@elasticmachine test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use getProject().file() here to be consistent with how Gradle resolves strings to files.
For relative paths, new File() is relative to the current working directory, while project.file() is relative to project.projectDir.
|
@elasticmachine test this please |
|
Yep I was using Files.writeString which exists since java 11... |
buildSrc/src/test/java/org/elasticsearch/gradle/FileContentsTaskTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, will wait for CI and merge if all goes well.
Thanks again for your contribution!
One last time, @elasticmachine test this please.
|
@elasticmachine test this please |
|
@baptistemesta please look at the CI failures. Checkstyle found some unused imports. @elasticmachine run packaging tests |
|
@elasticmachine run sample packaging tests |
|
@atorok Ok i'll fix that, thanks |
44541be to
9d73661
Compare
|
all good, fixed codestyle + squashed all commits |
|
@elasticmachine test this please |
|
@baptistemesta While not immediately obvious, the |
|
@baptistemesta I'm closing this due to lack of activity, please reopen if you plan to work on it, |
Relates to #34459
#34459