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

Added Money#to_json method #88

Closed
wants to merge 1 commit into from
Closed

Added Money#to_json method #88

wants to merge 1 commit into from

Conversation

rwz
Copy link

@rwz rwz commented Jun 17, 2011

No description provided.

@weppos
Copy link
Member

weppos commented Jun 17, 2011

Do we really need a to_json, to_xml, to_yaml, ... feature?

I'm not really sure they makes sense because users might want to use a different to_json logic depending on your application.

Also, it is worth to mention that, by default, the JSON library adds a to_json method at Object level. It means, every object is "jsonificable". The way it works by default, is to export all the internal object attributes.
Last but not least, a Money#to_json might probably require a Money.from_json counterpart.

From my very personal point of view, I would delegate the responsibility to create a to_json method to the final user.

@weppos
Copy link
Member

weppos commented Jun 17, 2011

Also, it's important to keep in mind that the JSON library is actually a gem and is not part of the Ruby Standard Library, at least in Ruby 1.8.7. What if I want to use Yajl instead of JSON? Or any other library? I would have to re-implement this method in any case.

@rwz
Copy link
Author

rwz commented Jun 17, 2011

Well, i saw JSON.dump in other parts of the code, for example in VariableExchange#export_rates. so i concluded it's safe to use it . Anyway, it can be easy implemented with only string interpolation.

I only implemented it because Object#to_json is way too verbose for Money object. It looks something like:

{"cents"=>1000,
 "currency"=>
  {"id"=>"usd",
   "priority"=>1,
   "iso_code"=>"USD",
   "name"=>"United States Dollar",
   "symbol"=>"$",
   "subunit"=>"Cent",
   "subunit_to_unit"=>100,
   "symbol_first"=>true,
   "html_entity"=>"$",
   "decimal_mark"=>".",
   "thousands_separator"=>","},
 "bank"=>{"rounding_method"=>nil, "rates"=>{}, "mutex"=>{}}}

@semmons99
Copy link
Member

I'm going to leave this to users to define. The only reason that banks have #to_json methods is to easily unload and load rates.

@semmons99 semmons99 closed this Jun 17, 2011
Losangelosgenetics pushed a commit to Losangelosgenetics/money that referenced this pull request Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants