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

Only rescue errors explicitly #239

Merged
merged 2 commits into from
Dec 23, 2015

Conversation

iainbeeston
Copy link
Contributor

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:

  • 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).

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).
@iainbeeston
Copy link
Contributor Author

Does anyone want to take a look at this?

@RST-J
Copy link
Contributor

RST-J commented Jun 2, 2015

Seems alright 👍

@iainbeeston
Copy link
Contributor Author

@RST-J Thanks! Do you think it's safe to merge?

@iainbeeston
Copy link
Contributor Author

I think with this and #255 it's worth making a minor release

@RST-J
Copy link
Contributor

RST-J commented Jun 3, 2015

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.

@iainbeeston
Copy link
Contributor Author

iainbeeston commented Jun 3, 2015 via email

@RST-J
Copy link
Contributor

RST-J commented Jun 3, 2015

I think we could add it right away, there is no reason to wait.

@mjc
Copy link
Contributor

mjc commented Jun 4, 2015

I'd very much appreciate both a minor release with this, #255, and a changelog. 👍 😄

iainbeeston added a commit that referenced this pull request Dec 23, 2015
@iainbeeston iainbeeston merged commit 9de1347 into voxpupuli:master Dec 23, 2015
@iainbeeston iainbeeston deleted the rescue-explicit-errors branch December 23, 2015 08:38
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.

3 participants