You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "FredrikAnderson (via GitHub)" <gi...@apache.org> on 2024/07/27 17:53:47 UTC
[PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
FredrikAnderson opened a new pull request, #202:
URL: https://github.com/apache/maven-site-plugin/pull/202
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MSITE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[MSITE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MSITE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694170614
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -219,14 +221,14 @@ public void davDeployThruProxyWitAuthzInProxy() throws Exception {
}
private void assertContentInFiles() throws Exception {
- File htmlFile = new File(siteTargetPath, "site" + File.separator + "index.html");
- assertTrue(htmlFile.exists());
- String fileContent = FileUtils.readFileToString(htmlFile, StandardCharsets.UTF_8);
+ File fileToTest = new File(siteTargetPath, "site" + File.separator + "index.html");
Review Comment:
Reonstantiate the encoding please
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -219,14 +221,14 @@ public void davDeployThruProxyWitAuthzInProxy() throws Exception {
}
private void assertContentInFiles() throws Exception {
- File htmlFile = new File(siteTargetPath, "site" + File.separator + "index.html");
- assertTrue(htmlFile.exists());
- String fileContent = FileUtils.readFileToString(htmlFile, StandardCharsets.UTF_8);
+ File fileToTest = new File(siteTargetPath, "site" + File.separator + "index.html");
Review Comment:
Reinstantiate the encoding please
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694344249
##########
src/test/java/org/apache/maven/plugins/site/deploy/SimpleDavServerHandler.java:
##########
@@ -111,4 +112,17 @@ public int getPort() {
public void stop() throws Exception {
server.stop();
}
+
+ private void writeByteArrayToFile(File targetFile, ServletInputStream inputStream) throws IOException {
Review Comment:
Adjusted.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694234300
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -219,14 +221,14 @@ public void davDeployThruProxyWitAuthzInProxy() throws Exception {
}
private void assertContentInFiles() throws Exception {
- File htmlFile = new File(siteTargetPath, "site" + File.separator + "index.html");
- assertTrue(htmlFile.exists());
- String fileContent = FileUtils.readFileToString(htmlFile, StandardCharsets.UTF_8);
+ File fileToTest = new File(siteTargetPath, "site" + File.separator + "index.html");
+ assertTrue(fileToTest.exists());
+ String fileContent = readFileToString(fileToTest);
assertTrue(fileContent.contains("Welcome to Apache Maven"));
- File cssFile = new File(siteTargetPath, "site" + File.separator + "css" + File.separator + "maven-base.css");
- assertTrue(cssFile.exists());
- fileContent = FileUtils.readFileToString(cssFile, StandardCharsets.UTF_8);
+ fileToTest = new File(siteTargetPath, "site" + File.separator + "css" + File.separator + "maven-base.css");
Review Comment:
Please don't reuse local variables for different things. It's very prone.
##########
src/test/java/org/apache/maven/plugins/site/deploy/SimpleDavServerHandler.java:
##########
@@ -111,4 +112,17 @@ public int getPort() {
public void stop() throws Exception {
server.stop();
}
+
+ private void writeByteArrayToFile(File targetFile, ServletInputStream inputStream) throws IOException {
+ int nRead;
+ byte[] data = new byte[512];
Review Comment:
larger buffer in 2024, maybe 4K
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -247,4 +249,26 @@ private boolean assertAtLeastOneRequestContainsHeader(List<HttpRequest> requests
}
return false;
}
+
+ private void cleanDirectory(File dir) throws IOException {
Review Comment:
This should be done with a JUnit tenmporary directory. No need to invent this code. I sent a PR for that.
##########
src/test/java/org/apache/maven/plugins/site/deploy/SimpleDavServerHandler.java:
##########
@@ -111,4 +112,17 @@ public int getPort() {
public void stop() throws Exception {
server.stop();
}
+
+ private void writeByteArrayToFile(File targetFile, ServletInputStream inputStream) throws IOException {
Review Comment:
writeInputStreamToFile
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o closed pull request #202: [MSITE-945] Remove dependency on Commons IO
URL: https://github.com/apache/maven-site-plugin/pull/202
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694343621
##########
src/test/java/org/apache/maven/plugins/site/deploy/AuthAsyncProxyServlet.java:
##########
@@ -137,4 +142,16 @@ public void service(ServletRequest req, ServletResponse res) throws ServletExcep
super.service(req, res);
}
+
+ private void writeByteArrayToFile(File targetFile, ServletInputStream inputStream) throws IOException {
Review Comment:
Why not https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#copy-java.io.InputStream-java.nio.file.Path-java.nio.file.CopyOption...-?
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -246,4 +248,26 @@ private boolean assertAtLeastOneRequestContainsHeader(List<HttpRequest> requests
}
return false;
}
+
+ private void cleanDirectory(File dir) throws IOException {
+ Files.list(dir.toPath())
+ .flatMap(p -> {
+ try {
+ return Files.walk(p);
+ } catch (IOException e) {
+ return Stream.empty();
+ }
+ })
+ .sorted(Comparator.reverseOrder())
+ .forEach(p -> {
+ try {
+ Files.delete(p);
+ } catch (IOException e) {
+ }
+ });
+ }
+
+ private String readFileToString(File fileToTest) throws IOException {
+ return Files.lines(fileToTest.toPath()).collect(Collectors.joining(System.lineSeparator()));
Review Comment:
I think that this would be more efficient: https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#readAllBytes-java.nio.file.Path-
##########
src/test/java/org/apache/maven/plugins/site/deploy/SimpleDavServerHandler.java:
##########
@@ -74,7 +75,7 @@ public void handle(String target, Request r, HttpServletRequest request, HttpSer
if (request.getMethod().equalsIgnoreCase("PUT")) {
File targetFile = new File(siteTargetPath, targetPath);
- FileUtils.writeByteArrayToFile(targetFile, IOUtils.toByteArray(request.getInputStream()));
+ writeInputStreamToFile(targetFile, request.getInputStream());
Review Comment:
Why not use https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#copy-java.io.InputStream-java.nio.file.Path-java.nio.file.CopyOption...-?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694344174
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -219,14 +221,14 @@ public void davDeployThruProxyWitAuthzInProxy() throws Exception {
}
private void assertContentInFiles() throws Exception {
- File htmlFile = new File(siteTargetPath, "site" + File.separator + "index.html");
- assertTrue(htmlFile.exists());
- String fileContent = FileUtils.readFileToString(htmlFile, StandardCharsets.UTF_8);
+ File fileToTest = new File(siteTargetPath, "site" + File.separator + "index.html");
Review Comment:
New method for reading file, readFileToString, using Files.lines, which default uses charset UTF-8 for reading.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#issuecomment-2254237447
See your point, fixed!
PR updated.
On Sat, Jul 27, 2024 at 8:37 PM Michael Osipov ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
>
> No blanket imports, please.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/maven-site-plugin/pull/202#pullrequestreview-2203220739>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGWA5SREPVZYX6ESZRQHULDZOPSFLAVCNFSM6AAAAABLSBVV5SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBTGIZDANZTHE>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694344213
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -247,4 +249,26 @@ private boolean assertAtLeastOneRequestContainsHeader(List<HttpRequest> requests
}
return false;
}
+
+ private void cleanDirectory(File dir) throws IOException {
Review Comment:
Done by you in other PR.
##########
src/test/java/org/apache/maven/plugins/site/deploy/AbstractSiteDeployWebDavTest.java:
##########
@@ -219,14 +221,14 @@ public void davDeployThruProxyWitAuthzInProxy() throws Exception {
}
private void assertContentInFiles() throws Exception {
- File htmlFile = new File(siteTargetPath, "site" + File.separator + "index.html");
- assertTrue(htmlFile.exists());
- String fileContent = FileUtils.readFileToString(htmlFile, StandardCharsets.UTF_8);
+ File fileToTest = new File(siteTargetPath, "site" + File.separator + "index.html");
+ assertTrue(fileToTest.exists());
+ String fileContent = readFileToString(fileToTest);
assertTrue(fileContent.contains("Welcome to Apache Maven"));
- File cssFile = new File(siteTargetPath, "site" + File.separator + "css" + File.separator + "maven-base.css");
- assertTrue(cssFile.exists());
- fileContent = FileUtils.readFileToString(cssFile, StandardCharsets.UTF_8);
+ fileToTest = new File(siteTargetPath, "site" + File.separator + "css" + File.separator + "maven-base.css");
Review Comment:
Fixed.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSITE-945] Remove dependency on Commons IO [maven-site-plugin]
Posted by "FredrikAnderson (via GitHub)" <gi...@apache.org>.
FredrikAnderson commented on code in PR #202:
URL: https://github.com/apache/maven-site-plugin/pull/202#discussion_r1694344292
##########
src/test/java/org/apache/maven/plugins/site/deploy/SimpleDavServerHandler.java:
##########
@@ -111,4 +112,17 @@ public int getPort() {
public void stop() throws Exception {
server.stop();
}
+
+ private void writeByteArrayToFile(File targetFile, ServletInputStream inputStream) throws IOException {
+ int nRead;
+ byte[] data = new byte[512];
Review Comment:
Change to 2k.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org