-
Notifications
You must be signed in to change notification settings - Fork 201
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
Psych::SyntaxError doesn't inherit from StandardError #23
Comments
Yes, this is expected behavior. It is a SyntaxError, so |
We too just tripped over this. As SyntaxError is descended from ScriptError and has siblings LoadError and NotImplementedError I'd always assumed it was for Ruby syntax errors only. I think of Psych as a library and was expecting any exceptions it raised to be caught by a rescue of StandardError. We're handling it now anyways, and I can see your point, but we did find it a bit surprising. |
We just got burned by this too. We had some "rescue Exception" to catch things like this, but that wreaked havoc in a multi-threaded environment. It wasn't the best code admittedly, but we tried to do the right thing and just catch StandardError. After three days of debugging, this issue was unearthed. I went to file the issue only to see it had already been closed. While it is a form of syntactical error, I don't think it's anywhere on par with a Ruby SyntaxError. I hope this practice isn't adopted en masse by other data format libraries because you can't reasonably rescue on any class higher up the ancestry without really screwing with Ruby. |
On one of my servers I updated to rails 3.2.3 and I get all these warnings whenever I run a rake task or rails console:
|
I agree that this is an incorrect use of SyntaxError. The literal description of a SyntaxError is "Raised when encountering Ruby code with an invalid syntax". What Psych is catching is malformed YAML. Different things. |
I completely agree with the other commenters saying that Ruby's built-in SyntaxError class is for Ruby code, not for just anything you might be trying to parse. This is evinced by the fact that SyntaxError is a subclass of ScriptError. My YAML is not a script—it is markup, despite having a somewhat chummier relationship with Ruby than other markup formats like JSON or XML. In your view, @tenderlove, is it incorrect to have one's parsing library's parsing exceptions subclass StandardError rather than SyntaxError, or is it just that you don't see any reason not to use SyntaxError since it's there and has such a convenient name? I've always coded by the rule of thumb that any exception classes I create or exceptions I explicitly raise should subclass StandardError, and that exceptions not descending from StandardError are reserved for the interpreter's use. I haven't noticed anything in any libraries that I use that run counter to that guideline except for Psych::SyntaxError. It's nice that Psych's parsing exceptions can indeed be explicitly rescued with |
I'm completely convinced of how incorrect I was. Can someone create a patch? |
For anyone bit by this issue, it was fixed in commit 66e22be from October 2012. The current rubygems.org release of psych 1.3.4 does not have this fix. @tenderlove Do you know at what point this change was picked up by Ruby 1.9 and/or 2.0? |
Is this expected behavior?
pysch_error_handling.rb
produces:
I also tried with the following Gemfile:
with this addition to the top of pysch_error_handling.rb:
which produces:
I guess this is because
Psych::SyntaxError
doesn't inherit fromStandardError
. Perhaps that is by design, but it caught me by surprise - so I thought I'd ask.The text was updated successfully, but these errors were encountered: