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 2020/04/10 11:27:03 UTC
[GitHub] [commons-io] ferenc-csaky opened a new pull request #111: [IO-661]
FileUtils throws inconsistent exceptions
ferenc-csaky opened a new pull request #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111
Changed `IllegalArgumentException` to `IOException` in is thrown, but in `copyDirectoryToDirectory(...)` and `copeFileToDirectory(...)` functions. As the Jira mentions, the move equivalent throws `IOException` already. Furthermore, every other intermittent method which is called also throws `IOException` for the same, so I think this is the good choice here.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] ferenc-csaky commented on issue #111: [IO-661]
FileUtils throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
ferenc-csaky commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-614341907
I totally agree with that IAE reflects better what is the actual problem. But I think that involves other layers.
For example, the `copyFileToDirectory(File, File, boolean)` function I modified calls the `copyFile(File, File, boolean)` function inside, which throws IOE if the expected `srcFile` is a directory. The next step in the method call chain is the private `doCopyFile(final File srcFile, final File destFile, final boolean preserveFileDate)` function, which actually does the operation and it also throws an IOE if `destFile` is a directory.
My point is basically every other function in this bunch throws IOE for these kind of checks. It applies to the whole `copyDirectoryToDirectory(...)` chain and to the move operations as well. So changing only the top layer would not solve the problem IMO.
Of course, I would happily go to the IAE direction, but to do that right it requires some other changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] garydgregory commented on issue #111: [IO-661]
FileUtils throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-612025177
Not sure about this one, needs some thought...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] garydgregory commented on issue #111: [IO-661]
FileUtils throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-614772094
IMO, the guideline should be:
- Use `Objects.requireNonNull()` for arg null-checks
- Use `IllegalArgumentException` for arg other-than-null checks
- Use `IOException` for things that need to be checked or are really IO issues.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] ecki commented on issue #111: [IO-661] FileUtils
throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
ecki commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-616210164
Generally I would agree with IAE for Argument checking however in the specific case an external condition (directory deleted or replaced by file) can cause the check to fail. Also the check methods hide IO exceptions so they can be intermittent. For this case I would prefer a change and think there should be no compatibility impact.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] michael-o commented on issue #111: [IO-661] FileUtils
throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-615838545
@garydgregory I have completely missed the missed! Thank you. I my opion this change is justified because you shall not pass invalid args from the beginning. We'd just cleaning up the API. If client code if correct they will never see any exception.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] garydgregory commented on issue #111: [IO-661]
FileUtils throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-615500548
-1: The approval (@michael-o) was premature since the build fails making this PR incomplete and not acceptable as is :-(
Changing the thrown exceptions requires the tests to match; authors (@ferenc-csaky), please run a local build before committing.
While the method signatures have not changed (we keep binary compatibility), the behavior has changed, this needs more thought IMO.
We need to really be sure that for each method, we document the changes in https://issues.apache.org/jira/browse/IO-661. The Javadoc has been updated so that's good.
We also need to convince ourselves that it is OK to break some call sites with this change. I am not convinced yet.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-io] garydgregory commented on issue #111: [IO-661]
FileUtils throws inconsistent exceptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #111: [IO-661] FileUtils throws inconsistent exceptions
URL: https://github.com/apache/commons-io/pull/111#issuecomment-612625270
-1. I agree with @michael-o , throwing an IAE is the proper way to go here.
----------------------------------------------------------------
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
With regards,
Apache Git Services