MRI: Actually throw ENOENT for missing files

mook commented

2015-10-29 04:40:49 +00:00

(Migrated from github.com)

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 :|

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): ``` ruby 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 commented

2015-10-30 03:31:46 +00:00

(Migrated from github.com)

Thanks for the feedback! Not sure why GitHub seems to have lost it; it can be found at mook/mkxp@75f6a8c372. 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.

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.

Ancurio commented

2015-10-30 09:28:36 +00:00

(Migrated from github.com)

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

What other error codes are wrong?

Ah, sorry, I must have commented on the commit in your fork instead of in this RP. What other error codes are wrong?

mook commented

2015-10-30 14:49:18 +00:00

(Migrated from github.com)

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.

Ancurio commented

2015-10-30 15:18:48 +00:00

(Migrated from github.com)

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

Success
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.

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.

mook commented

2015-10-31 03:46:31 +00:00

(Migrated from github.com)

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

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

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.

git checkout -b mook/filesystem-enoent master

git pull origin mook/filesystem-enoent

Step 2:

Merge the changes and update on Forgejo.

git checkout master

git merge --no-ff mook/filesystem-enoent

git push origin master

MRI: Actually throw ENOENT for missing files #137

Step 1:

Step 2: