-
Notifications
You must be signed in to change notification settings - Fork 32
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
Repair json parsing issues. #49
Conversation
CHANGELOG.md
Outdated
@@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
|
|||
- Your contribution here! | |||
|
|||
### Bugfix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Fixed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's been fixed
lib/drip/client.rb
Outdated
@@ -116,7 +116,9 @@ def make_request(verb_klass, uri, options, step = 0) | |||
|
|||
def build_response(&block) | |||
response = yield | |||
Drip::Response.new(response.code.to_i, response.body) | |||
Drip::Response.new(response.code.to_i, response.body ? JSON.parse(response.body) : nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could response.body
be an empty string? If so, could use present?
. Or could response.body
every be false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty string triggers the JSON::ParserError
and returns nil
, which is correct.
It can't be false, and if it were, the result should be nil
.
#present?
comes from ActiveSupport, which we don't use here.
I've improved the empty string case to not require an exception, and added a test (which passed before the change).
Fixes #48