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
ZOOKEEPER-3836: Use Commons and JDK Functions in ClientBase #1358
Conversation
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.
+1, but please create one ticket for same refactors next time.
@@ -64,7 +66,7 @@ public void testWriteNewFile() throws IOException { | |||
fos.close(); | |||
assertTrue(dstFile.exists()); | |||
|
|||
String readBackData = ClientBase.readFile(dstFile); | |||
String readBackData = FileUtils.readFileToString(dstFile, StandardCharsets.UTF_8); |
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.
UTF_8
is one thing that can be really useful to have a static import, for readability.
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.
Also, there are many new ways in Java 7/8 to read from a file. I'm not sure it is necessary to rely on Commons for this. Keeping the number of uses of Commons down means that it may be easier to drop as a dependency in future, as functions are provided directly by Java itself. Fewer dependencies means greater stability in the long run, so if ZK doesn't need to use a dependency, it shouldn't.
Here are some ways to read from a File without commons, found by using Google: https://howtodoinjava.com/java/io/java-read-file-to-string-examples/
We probably want something like:
String readBackData = new String(Files.readAllBytes(dst.toPath()), UTF_8);
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.
Good point on the apache commons usage. Honestly, didn't even realised it's from there, I am also used to FileUtils so much...
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. Made the changes.
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.
@belugabehr You only changed it in one place. I only commented once, but there were many lines that were changed in the same way in the PR.
Sorry for the churn. My coffee has NOT kicked in this morning obviously. |
2f12973
to
ff9236f
Compare
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.
LGTM
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.
Still looks good to me.
Merged to master, thanks @belugabehr |
Related to apache#1357 Author: David Mollitor <dmollitor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1358 from belugabehr/ZOOKEEPER-3836
Related to apache#1357 Author: David Mollitor <dmollitor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1358 from belugabehr/ZOOKEEPER-3836
Related to apache#1357 Author: David Mollitor <dmollitor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1358 from belugabehr/ZOOKEEPER-3836
Related to apache#1357 Author: David Mollitor <dmollitor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1358 from belugabehr/ZOOKEEPER-3836
Related to #1357