Introduce from_json as the inverse of to_json #326
Conversation
lib/money/money.rb
Outdated
| @@ -74,6 +75,16 @@ def from_subunits(subunits, currency_iso, format: :iso4217) | |||
| new(value, currency) | |||
| end | |||
|
|
|||
| def from_json(hash_or_string) | |||
There was a problem hiding this comment.
I wonder if this might be a bit easier to follow with explicit, separate methods from_hash and from_json that each handle a single data type, rather than attempting to juggle types within a single method. It seems a slightly surprising result to have a method from_json that accepts an input which is not JSON.
There was a problem hiding this comment.
The concern that from_json takes something that's not a JSON object is valid, but it's an ambiguity that already exists in the gem in to_json: void -> String vs as_json: void -> Hash.
I don't have a strong opinion but my line of thought was that the utility of a from_json that understands both JSON-encoded strings and JSON-encoded hashes was greater than having separate methods, which would leave a user guessing "which one do I have to use again?"
There was a problem hiding this comment.
If we look at how rails handles these:
to_json - JSON string representing a model
as_json - Hash representing a model
from_json - Sets the model attributes from a JSON string.
Based on convention I agree that from_json should expect a json String and if we needed to convert a hash we should have a from_hash.
Especially given our use of Sorbet I would expect the user to be aware if they needed one or the other.
There was a problem hiding this comment.
Let's start only with from_json (with a json string). Going from a hash to a Money object is trivial Money.new(hash[:amount], hash[:currency]) and doesn't really require an extra helper.
lib/money/money.rb
Outdated
| @@ -74,6 +75,16 @@ def from_subunits(subunits, currency_iso, format: :iso4217) | |||
| new(value, currency) | |||
| end | |||
|
|
|||
| def from_json(hash_or_string) | |||
There was a problem hiding this comment.
If we look at how rails handles these:
to_json - JSON string representing a model
as_json - Hash representing a model
from_json - Sets the model attributes from a JSON string.
Based on convention I agree that from_json should expect a json String and if we needed to convert a hash we should have a from_hash.
Especially given our use of Sorbet I would expect the user to be aware if they needed one or the other.
|
@pedropb if you want to add just |
|
Hey folks, pushed some rebased commits. Here are the changes:
|
The Money gem provides two JSON generating methods:
to_jsongenerates a JSON-encoded String representation of the Money object.as_jsongenerates a JSON-encoded Hash representation of the Money object.This PR introduces a class method that accepts any of these 2 JSON representations and generates a Money object from it.