Adapt to new Grape::Exceptions::ErrorResponse throwable - #98
Conversation
Grape v3.3.0 throws `Grape::Exceptions::ErrorResponse` objects instead of hashes, so we adjust accordingly while also maintaining backwards compatibility. This case was not covered by the test suite, so we add a matching test. Change in the `grape` gem: - [grape v3.3.0 changelog entry](https://github.com/ruby-grape/grape/blob/v3.3.0/CHANGELOG.md?plain=1#L22) - [grape pull request](ruby-grape/grape#2693) - [grape commit in master branch](ruby-grape/grape@c464ffa)
samsonjs
left a comment
samsonjs
left a comment
There was a problem hiding this comment.
Thanks for the fix! Looks good overall and thanks for adding a test. I have a nitpicky request but otherwise this is good to go.
|
|
||
| def status_and_message(error) | ||
| case Gem::Version.new(Grape::VERSION) | ||
| when Gem::Requirement.new('>= 3.3.0') |
There was a problem hiding this comment.
I realize that we can't name the new error class without blowing up on old versions of grape, but how about checking whether error.is_a?(Hash) instead of the explicit version check? I think that should do the trick too and then we don't have to rely on specific versions. When it's not a hash we'll try error.status and if something changes in the future it'll blow up in the same way, so I think it preserves the behaviour you have here.
There was a problem hiding this comment.
@samsonjs Before I do that, please consider the following: I frequently post PRs for gems which need to support a wide range of versions of another gem (especially activerecord). Depending on the churn in that dependency, you might need to adapt more or less often. Example: HairTrigger.migrator
Using the case statement with Gem::Version and Gem::Requirement has proven to be the best practice here because:
- It uses the same version comparison as Bundler, so you are sure to have the correct versions in place
- It is concise (compared to the
if-elsif-elsif-endsome people like to use) - and most of all: The code clearly documents for posteriority which conditional branches can be deleted without loss once you drop support for older versions of the dependency. No guesswork!
So, do you really want me to remove this?
There was a problem hiding this comment.
Thanks for giving your perspective on this. I'm sold especially about cleanup later on after dropping support for 3.3 when we've long forgotten about this PR. Will merge it as is.
3 participants
Grape v3.3.0 throws
Grape::Exceptions::ErrorResponseobjects instead of hashes, so we adjust accordingly while also maintaining backwards compatibility.This case was not covered by the test suite,
so we add a matching test.
Change in the
grapegem: