commons-io icon indicating copy to clipboard operation
commons-io copied to clipboard

[IO-808] Report problems in the file system as IOExceptions

Open elharo opened this issue 1 year ago • 1 comments
trafficstars

Problems that can't be determined from the argument alone, that is problems that aren't detected until the file system is read, are IOExceptions, not program bugs represented by IllegalArgumentException. No API changes. @garydgregory

elharo avatar Feb 09 '24 04:02 elharo

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.12%. Comparing base (858f5cf) to head (e0d6da0). Report is 927 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #586      +/-   ##
============================================
+ Coverage     86.01%   86.12%   +0.11%     
+ Complexity     3499     3498       -1     
============================================
  Files           231      231              
  Lines          8250     8244       -6     
  Branches        960      960              
============================================
+ Hits           7096     7100       +4     
+ Misses          864      854      -10     
  Partials        290      290              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Feb 09 '24 04:02 codecov-commenter

Quoting from the Javadoc of IllegalArgumentException: "Thrown to indicate that a method has been passed an illegal or inappropriate argument."

In other words, throwing an IAE is perfectly alright, there is no condition on how to detect the fact, that is the argument is illegal, or inappropriate.

I conclude, that this patch adds nothing to the existing code, at the cost of changing method signatures. I will not merge it.

jochenw avatar May 17 '24 12:05 jochenw

Closing, I agree with @jochenw .

garydgregory avatar May 17 '24 13:05 garydgregory

When the problem is detected a core difference between runtime and checked exceptions. Runtime exceptions indicate program bugs, like passing the wrong argument. Checked exceptions indicate environmental problems that are not known at compile time.

A file that does not exist is not an illegal or inappropriate argument. It's not the argument that's wrong. It's the file system. Consider this.

The file could exist when the argument is passed and then be deleted by a different process before the method tries to write or read it. The argument was 100% correct but there's still an exception. That shouldn't be an IllegalArgumentException. The argument wasn't the problem he I/O was.

elharo avatar May 19 '24 11:05 elharo

Quoting @elharo: "A file that does not exist is not an illegal or inappropriate argument. "

That is, in my opinion, a matter of policy, hence questionable.

My personal opinion is this: It is the callers responsibility to ensure, that a file exists, before invoking an operation like copyFile, or moveFile. How that is ensured, is up to the caller. However, if you adopt that opinion, then it is clearly an illegal argument.

Whatever, I do not wish to continue this discussion. In the end, it is pointless. The current code works, as documented. (@throws IAE.) Even if we agreed on your policies, I'd still hold to the statement "Works as documented (perhaps not exactly as expected by you), so no need to fix anything."

jochenw avatar May 19 '24 12:05 jochenw

The "policy" is set by the design of Java since 1.0. The current approach fights against how exceptions were designed to work in Java. Sure, you can do it that way and it compiles and perhaps is documented, but it's more than little like building a car where first gear is full highway cruising speed and fourth gear is for pulling a horse trailer up a mountain. It's going to confuse everyone who tries to drive it and probably cause a few accidents, and documenting this in the owners' manual really wouldn't help.

elharo avatar May 19 '24 23:05 elharo