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

Make include_json an identity function #61

Closed
wants to merge 3 commits into from
Closed

Make include_json an identity function #61

wants to merge 3 commits into from

Conversation

myitcv
Copy link

@myitcv myitcv commented Nov 29, 2013

To effectively support the following (which is also in a test):

it "is an identity function" do
  json = %({"one":1})
  json.should include_json(%({"one":1}))
end

@charlierudolph
Copy link

I'm not a big fan of this. Can you please give an example where you needed this? Maybe there is a different approach.

I'd like to keep include_json in line with Ruby Enumerable module's include?
We follow the pattern for strings and arrays, though not for hashes (that probably should change).

Also when looking up identity function, its definition is a function that returns its input.

@myitcv
Copy link
Author

myitcv commented Dec 2, 2013

Wow, I was clearly doing too many things at once when submitting this.

You can almost certainly tell from the code that I intended for include_json to obey the rules of a subset relation. Definitely not an identity function!

I can understand if you want to keep include_json more in line with Ruby Enumerable semantics and terminology.

But on the basis that a JSON object is more like a set of attribute-value pairs, you can see where I'm coming from?

I will close this PR on the basis the name of the branch is clearly bonkers.... and then reference the new PR from here.

@myitcv myitcv closed this Dec 2, 2013
@myitcv myitcv deleted the make_include_json_identity branch December 2, 2013 08:48
@myitcv
Copy link
Author

myitcv commented Dec 2, 2013

Recreated as #62

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.

2 participants