You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/04/09 15:48:29 UTC
[GitHub] [commons-io] arturobernalg opened a new pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
arturobernalg opened a new pull request #221:
URL: https://github.com/apache/commons-io/pull/221
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-816807362
[](https://coveralls.io/builds/39922504)
Coverage remained the same at 89.265% when pulling **245cb08d942a7a5242b686cc8d131cfe049ed130 on arturobernalg:feature/newOutputStream** into **651496937d094d5dabd166897136f934ed573c7a on apache:master**.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-816807362
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a change in pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #221:
URL: https://github.com/apache/commons-io/pull/221#discussion_r633126756
##########
File path: src/test/java/org/apache/commons/io/IOUtilsTestCase.java
##########
@@ -118,7 +116,7 @@ public void setUp() {
if (!testFile.getParentFile().exists()) {
throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
- final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
+ final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Review comment:
In these test methods, you keep calling `toPath()` over and over, why not just call it once? IOW should `testFile` be `testPath` or have both?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory merged pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #221:
URL: https://github.com/apache/commons-io/pull/221
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls commented on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-816807362
[](https://coveralls.io/builds/38683965)
Coverage increased (+0.01%) to 89.275% when pulling **7d158e6228a61b97c223cd1626da1b679deb211e on arturobernalg:feature/newOutputStream** into **f5de47f2fda3bb8a3fd2daf06b431282f40e3fa8 on apache:master**.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] arturobernalg commented on a change in pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #221:
URL: https://github.com/apache/commons-io/pull/221#discussion_r633231050
##########
File path: src/test/java/org/apache/commons/io/IOUtilsTestCase.java
##########
@@ -118,7 +116,7 @@ public void setUp() {
if (!testFile.getParentFile().exists()) {
throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
- final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
+ final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Review comment:
HI @garydgregory
you're right. Changed.
TY
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-816807362
[](https://coveralls.io/builds/39729613)
Coverage decreased (-0.1%) to 89.27% when pulling **186611e8a43bdcd8b5829f0707b900530a2f06a9 on arturobernalg:feature/newOutputStream** into **50bfa728a8852149cfe33bf7e53136e269ec9b74 on apache:master**.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-855947946
>
>
> > Files.newInputstream() to new FileInputStream()
>
> HI @jochenw
> This is the reason -->
> 'Basically, FileInputStream and FileOutputStream both use a finalizer to ensure that the resources are closed.
> Even with programmers do call close() after using FileInputStream, its finalize() method will still be called. In other words, still get the side effect of the FinalReference being registered at FileInputStream allocation time, and also reference processing to reclaim the FinalReference during GC (any GC solution has to deal with this).'
Indeed, @jochenw do notice `java.io.FileInputStream.finalize()` and `java.io.FileOutputStream.finalize()`. I'll review further but I am incline to merged based on the `finalize()` issue. While in the long term I think finalization is going away, we are still on Java 8 for now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] arturobernalg commented on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-846767758
> Files.newInputstream() to new FileInputStream()
HI @jochenw
This is the reason -->
'Basically, FileInputStream and FileOutputStream both use a finalizer to ensure that the resources are closed.
Even with programmers do call close() after using FileInputStream, its finalize() method will still be called. In other words, still get the side effect of the FinalReference being registered at FileInputStream allocation time, and also reference processing to reclaim the FinalReference during GC (any GC solution has to deal with this).'
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] arturobernalg commented on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-840535967
> @arturobernalg
> May you please rebase on master which should allow the Java 17-EA build to pass.
HI @garydgregory
Done.
TY
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a change in pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #221:
URL: https://github.com/apache/commons-io/pull/221#discussion_r633126756
##########
File path: src/test/java/org/apache/commons/io/IOUtilsTestCase.java
##########
@@ -118,7 +116,7 @@ public void setUp() {
if (!testFile.getParentFile().exists()) {
throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
- final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
+ final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Review comment:
In these test methods, you keep calling `toPath()` over and over, why not just call it once?
##########
File path: src/test/java/org/apache/commons/io/IOUtilsTestCase.java
##########
@@ -118,7 +116,7 @@ public void setUp() {
if (!testFile.getParentFile().exists()) {
throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
- final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
+ final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Review comment:
In these test methods, you keep calling `toPath()` over and over, why not just call it once? IOW should `testFile` be `testPath` or have both?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] jochenw commented on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
jochenw commented on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-846531377
I am unsure about this one. First of all, it changes way too many files, in my opinion.
Then, I see no real benefit, when comparing Files.newInputstream() to new FileInputStream(). As far as I know, there is no real difference between those streams. In particular, neither is buffered.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-816807362
[](https://coveralls.io/builds/39729675)
Coverage decreased (-0.1%) to 89.255% when pulling **186611e8a43bdcd8b5829f0707b900530a2f06a9 on arturobernalg:feature/newOutputStream** into **50bfa728a8852149cfe33bf7e53136e269ec9b74 on apache:master**.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a change in pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #221:
URL: https://github.com/apache/commons-io/pull/221#discussion_r633126756
##########
File path: src/test/java/org/apache/commons/io/IOUtilsTestCase.java
##########
@@ -118,7 +116,7 @@ public void setUp() {
if (!testFile.getParentFile().exists()) {
throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
- final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
+ final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Review comment:
In these test methods, you keep calling `toPath()` over and over, why not just call it once?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #221:
URL: https://github.com/apache/commons-io/pull/221#issuecomment-840533354
@arturobernalg
May you please rebase on master which should allow the Java 17-EA build to pass.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] arturobernalg commented on a change in pull request #221: Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #221:
URL: https://github.com/apache/commons-io/pull/221#discussion_r633231050
##########
File path: src/test/java/org/apache/commons/io/IOUtilsTestCase.java
##########
@@ -118,7 +116,7 @@ public void setUp() {
if (!testFile.getParentFile().exists()) {
throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
- final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
+ final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Review comment:
HI @garydgregory
you're right. Changed.
TY
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org