-
Notifications
You must be signed in to change notification settings - Fork 680
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
Makes JSON resource enumerable, despite method_missing magic #2910
Makes JSON resource enumerable, despite method_missing magic #2910
Conversation
Signed-off-by: David Alexander <opensource@thelonelyghost.com>
Thanks @TheLonelyGhost ! This looks pretty slick. You mind adding a unit test for this? |
@jquick I believe I did under |
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.
Thanks @TheLonelyGhost !
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.
This is great, just have a couple of questions. =^)
@@ -37,6 +38,9 @@ def initialize(opts) | |||
# load the raw content from the source, and then parse it | |||
@raw_content = load_raw_content(opts) | |||
@params = parse(@raw_content) | |||
|
|||
# If the JSON content is enumerable, make this object enumerable too | |||
extend EnumerableDelegation if @params.respond_to?(:each) |
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.
Reading above, and parse()
, it looks like there should always be a valid hash for @params
, what's the scenario you have in mind where this condition would 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.
I originally considered json(content: 'null')
or something similar that is neither an array nor a JSON object. Upon actually testing this just now, I see that JSON.parse()
actually fails with anything other than those two. That was news to me.
From the look of things "valid JSON" was once defined as either an array or object, but ECMAScript 5 expanded it to include any data type such as "fizz buzz"
, null
, 3.14
, and others. It doesn't appear ruby's JSON parser has caught up yet.
At this point, this line doesn't really hurt anything, but builds in support for when and if the JSON parser is updated to the ES5 standard definition of "valid" JSON.
@@ -0,0 +1,9 @@ | |||
# encoding: utf-8 | |||
|
|||
module EnumerableDelegation |
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.
This is purpose built for params instead of delegation to some injected receiver, so I wonder if Params should be part of the name? Maybe EnumerableParamsDelegator
?
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 didn't like the dependency on the params attribute either, but minimum viable solution and all. I agree that your suggestion is a better name, but I'm definitely more interested in if you have a solution that delegates to an injected receiver. Ideas?
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.
Seems sensible, and the test makes sense and passes. Thanks, @TheLonelyGhost
Per Slack discussion:
Above does not respond as expected due to method_missing magic. One expects 2 tests to execute, but each does not iterate over its data. This PR corrects that and includes all of the same enumerable abilities on the
json
(and by extension,yaml
) resource that its data contains.Yes, I am aware this could also be accomplished by doing
obj.params.each do
instead ofobj.each do
, but I don't feel this is very intuitive. We're exposing the:[]
method via method_missing, why not make it react like a hash or array too?