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

Corrected Misspelling - Add .to_json to explicit conversions #23

Closed
wants to merge 3 commits into from

Conversation

jkogara
Copy link

@jkogara jkogara commented May 31, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2ac750d on johnogara:master into e740fc4 on avdi:master.

@avdi
Copy link
Owner

avdi commented May 31, 2013

I'm iffy on this. I don't think of #to_json as a "core" conversion, since it's something that only appears if you load certain libraries.

Talk me into it?

@jkogara
Copy link
Author

jkogara commented Jun 1, 2013

Ok, I understand why you may be iffy on this one. I guess one gets used to thinking of #to_json being a "core" conversion when working predominantly in a rails environment.
You open to additional conversions being added based which libraries are included in the environment?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8c41d90 on johnogara:master into e740fc4 on avdi:master.

@codeodor
Copy link

codeodor commented Jun 1, 2013

I won't weigh in on whether it should be included simply because of Rails, but I do have a couple of suggestions:

  1. Consider overriding as_json(options={}) instead.

While Rails will call to_json, as_json is the preferred way to structure the JSON as Ruby, and then to_json knows what to do with it automatically.

It might not be a common thing, but you might have someone expect as_json to work, call it first, then call to_json. As implemented now, here's what they'd see:

1.9.3p194 :003 > class NullObject
1.9.3p194 :004?>   def to_json
1.9.3p194 :005?>     "null"
1.9.3p194 :006?>     end
1.9.3p194 :007?>   end
 => nil 
1.9.3p194 :008 > NullObject.new.to_json
 => "null" 
1.9.3p194 :009 > NullObject.new.as_json
 => {} 
1.9.3p194 :010 > NullObject.new.as_json.to_json
 => "{}" 

In this case, they'd end up with an empty object instead of "null".

  1. What about adding a railtie that defines Rails specific code, so it would only get loaded in the context of a Rails app?

@codeodor
Copy link

codeodor commented Jun 1, 2013

Ok, so I will weigh in on it. I think Rails apps would represent a significant chunk of the usage for gems like this, so it'd be nice to support such common conversions.

I'd rather see it outside of the main code though, so it doesn't get littered for people who aren't using it in such a manner.

If there were a lot of Rails differences to support, maybe a naught-rails gem would be called for, but for a single common conversion, that might be overkill. 😄

@jkogara
Copy link
Author

jkogara commented Jun 1, 2013

Thanks for the feedback. I agree that Rails apps will most likely represent a significant chunk of usage, will make some changes and come back.

@jkogara jkogara closed this Jun 1, 2013
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.

4 participants