Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRI: Actually throw ENOENT for missing files #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mook
Copy link
Contributor

@mook mook commented Oct 29, 2015

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
Copy link
Contributor Author

mook commented Oct 30, 2015

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

Ancurio commented Oct 30, 2015

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

What other error codes are wrong?

@mook
Copy link
Contributor Author

mook commented Oct 30, 2015

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
Copy link
Owner

Ancurio commented Oct 30, 2015

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
Copy link
Contributor Author

mook commented Oct 31, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants