-
Notifications
You must be signed in to change notification settings - Fork 17
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
Remove json dependency. JSONAPI.parse now only accepts a hash. #37
Remove json dependency. JSONAPI.parse now only accepts a hash. #37
Conversation
f8a24d9
to
f733239
Compare
rebased |
@@ -23,6 +23,7 @@ module JSONAPI | |||
# objects in the primary data must have an id. | |||
# @return [JSONAPI::Parser::Document] | |||
def parse(document, options = {}) | |||
raise Parser::InvalidDocument, 'document cannot be nil' if document.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.
I'm thinking that we might as well go the extra mile and require document
to be a Hash
.
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.
updated!
@@ -94,4 +94,10 @@ | |||
expect(document.data.first.relationships.author.data.to_hash).to eq @author_data_hash | |||
expect(document.data.first.relationships.comments.data.map(&:to_hash)).to eq @comments_data_hash | |||
end | |||
|
|||
it 'raises InvalidDocument on nil input' do |
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.
👍
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.
LGTM. Just update the yarddoc above the parse
method (# @param document [Hash, String] the JSON API document.
) and we're good to merge.
It also made the json.parse method not required, so I removed that, too |
|
||
Parser::Document.new(hash, options) | ||
raise Parser::InvalidDocument, 'document must be a hash' unless document.is_a?(Hash) | ||
Parser::Document.new(document, options) |
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.
Right, then you can also remove the dependency to json
in both this file and the gemspec.
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.
done
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.
LGTM
woo! |
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.
Oops, update README.md first.
Which part needs to be updated? |
My bad, no update needed. LGTM, rebase and we can merge. |
42ba2c9
to
01e5ff8
Compare
rebased |
Superseds #40, fixes #33, #35.