-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Only rescue errors explicitly #239
Only rescue errors explicitly #239
Conversation
There are a lot of places in the code where we rescue any error, at all. That's quite dangerous, as it could conceal bugs. I've changed it so that we always specify which error class we want to catch. Mostly it's been a minor change, but there are two API changes: * When it comes to loading data I've had to introduce an explicit list of protocols which we can load json over (otherwise it's possible to have a uri with a scheme that makes no sense - it'd still be a valid uri but loading from it is impossible). I've used the list of protocols that addressable recognizes for now. * No matter what JSON parser you use, we now always raise a JSON::Schema::JsonParseError. Without doing this it would be tricky to handle parser errors identically, for all parsers (the other option would be to catch a long list of potential parse errors, but this seems more sensible).
f0d72fd
to
cb52269
Compare
Does anyone want to take a look at this? |
Seems alright 👍 |
@RST-J Thanks! Do you think it's safe to merge? |
I think with this and #255 it's worth making a minor release |
I think so, the changes make exception handling more specific which is what we wanted here and as the tests pass, there is at least no break with expected behavior in the non-error cases. But as there are changes in how errors must be handled I suggest to add an note in the changelog and maybe also hints to the readme about how to deal with schema load errors and the ne JSON parse error before we make a release. |
Good idea.
Incidentally, we don't have a changelog, do we? (I was thinking that we
should add one starting with v3 but maybe there's no reason to wait?)
|
I think we could add it right away, there is no reason to wait. |
I'd very much appreciate both a minor release with this, #255, and a changelog. 👍 😄 |
632a9e6
to
a4b6677
Compare
a4b6677
to
31a620b
Compare
Only rescue errors explicitly
Only rescue errors explicitly
There are a lot of places in the code where we rescue any error, at all.
That's quite dangerous, as it could conceal bugs.
I've changed it so that we always specify which error class we want to
catch. Mostly it's been a minor change, but there are two API changes:
of protocols which we can load json over (otherwise it's possible to
have a uri with a scheme that makes no sense - it'd still be a valid
uri but loading from it is impossible). I've used the list of
protocols that addressable recognizes for now.
JSON::Schema::JsonParseError. Without doing this it would be tricky to
handle parser errors identically, for all parsers (the other option
would be to catch a long list of potential parse errors, but this
seems more sensible).