mkxp icon indicating copy to clipboard operation
mkxp copied to clipboard

MRI: Actually throw ENOENT for missing files

Open mook opened this issue 9 years ago • 5 comments

The wrong exception (EOFError instead of Errno::ENOENT) was being raised when load_data() was being passed an invalid path.

Test case (global scope in Main should do):

begin
  load_data("Data/not/a/real/file")
rescue Errno::ENOENT
  puts "PASS"
rescue EOFError
  puts "FAIL"
end

Yes, I actually found a game that did that, and handled Errno:ENOENT :|

mook avatar Oct 29 '15 04:10 mook

Thanks for the feedback! Not sure why GitHub seems to have lost it; it can be found at mook/mkxp@75f6a8c372f8e2769c1371d3c0affba1569bd223. Made an attempt at fixing the other error codes too, but I assume they could use improvement. Checked that the test case above still passes.

mook avatar Oct 30 '15 03:10 mook

Ah, sorry, I must have commented on the commit in your fork instead of in this RP.

What other error codes are wrong?

Ancurio avatar Oct 30 '15 09:10 Ancurio

Sorry, I just meant that I tried to sort the other physfs error codes in the same spot to other Ruby exceptions but am not confident I'm translating those correctly. I also suspect that some of the things I'm checking for can't actually occur at that point. At worst though it just means a different exception than expected.

mook avatar Oct 30 '15 14:10 mook

There are exactly two expected cases when the call comes from load_data (ie. code we don't control):

  1. Success
  2. File does not exist.

Looking at the physfs header, the 2nd case would be covered by PHYSFS_ERR_NOT_FOUND and PHYSFS_ERR_NOT_A_FILE. Every other error is irrelevant to the user script, and points to a mkxp-internal problem instead. Since we can't deal with it in a proper fashion, we just throw a generic Exception::PHYSFSError with the error string provided by physfs. The ruby code is not expected to be able to deal with it, but at least the user can report the error and it can be investigated. I've never encountered this case though.

Ancurio avatar Oct 30 '15 15:10 Ancurio

Okay, changed to handle just those two errors. Left it as a switch because it looked cleaner than an if to me.

mook avatar Oct 31 '15 03:10 mook