[PATCH] Represent currencies as Currency object #4

Closed
weppos opened this Issue Apr 15, 2010 · 28 comments

Comments

Projects
None yet
4 participants
@weppos
Member

weppos commented Apr 15, 2010

In my money fork I added the ability to represent currencies with Money::Currency.
http://github.com/weppos/money

Additional changes:

  • definition for almost all existing currencies
  • changed the documentation to reflect the Currency feature
  • added Money#symbol, Money#delimiter and Money#separator convenient methods
  • added deprecation warning whenever applicable

All commits are submitted along with a corresponding test suite.

I hope you would merge my changes to the mainstream repository.
I worked hard to ensure backward-compatibility with existing API.

-- Simone

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 15, 2010

Member

Thanks for the patch. I'll take a look at it and see about incorporating it into trunk.

Member

semmons99 commented Apr 15, 2010

Thanks for the patch. I'll take a look at it and see about incorporating it into trunk.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 15, 2010

Member

I have no problem merging this if you make one change. The Brazilian Real should always have a space between the symbol and the amount. You can see we had to patch this with the following commit 898d462. Once that is updated, I will merge your changes into trunk.

Member

semmons99 commented Apr 15, 2010

I have no problem merging this if you make one change. The Brazilian Real should always have a space between the symbol and the amount. You can see we had to patch this with the following commit 898d462. Once that is updated, I will merge your changes into trunk.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Apr 15, 2010

Member

Great!

I updated the repository. Make sure to destroy/recreate the remote source if you already fetched it because I used a rebase strategy to clean the commit history.

Thanks for your quick feedback!

Member

weppos commented Apr 15, 2010

Great!

I updated the repository. Make sure to destroy/recreate the remote source if you already fetched it because I used a rebase strategy to clean the commit history.

Thanks for your quick feedback!

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 15, 2010

Member

I've merged your changes into trunk. I've sent a message to Hong Lai, and as long as he doesn't object, I will release a new version of the gem.

Member

semmons99 commented Apr 15, 2010

I've merged your changes into trunk. I've sent a message to Hong Lai, and as long as he doesn't object, I will release a new version of the gem.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Apr 15, 2010

Member

Excellent!

I wish that all projects have this reactivity.
Looking forward to upgrading the Gem dependency list in my app. ;)

Thank you!

-- Simone

Member

weppos commented Apr 15, 2010

Excellent!

I wish that all projects have this reactivity.
Looking forward to upgrading the Gem dependency list in my app. ;)

Thank you!

-- Simone

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Apr 15, 2010

Contributor

Looks good, excellent work! Thanks for contributing. The only thing that I don't understand is the priority thing in Currency. It'd be great if you can document that in more detail. Other than that, feel free to release.

Contributor

FooBarWidget commented Apr 15, 2010

Looks good, excellent work! Thanks for contributing. The only thing that I don't understand is the priority thing in Currency. It'd be great if you can document that in more detail. Other than that, feel free to release.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Apr 16, 2010

Member

No problem.

I added a few more documentation in the following 2 changesets in my fork.
http://github.com/weppos/money/compare/cce64fa...07d18b2

The changes should apply cleanly in your fork queue. If not, let me know, I'll rebase them on your repos.

Member

weppos commented Apr 16, 2010

No problem.

I added a few more documentation in the following 2 changesets in my fork.
http://github.com/weppos/money/compare/cce64fa...07d18b2

The changes should apply cleanly in your fork queue. If not, let me know, I'll rebase them on your repos.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 16, 2010

Member

The updated gem has been released!

Member

semmons99 commented Apr 16, 2010

The updated gem has been released!

@spatrik

This comment has been minimized.

Show comment
Hide comment
@spatrik

spatrik Apr 29, 2010

I think patch broke the composed_of example for using the money gem with rails.

In my model I have:

composed_of :price,
  :class_name => "Money",
  :mapping => [%w(cents cents), %w(currency currency)]

Saving a variant object works, but when I load it back from the database, I get a YAML string instead:

 v2.currency # => "--- !ruby/object:Money::Currency \nid: :sek\niso_code: SEK\nname: Swedish Krona\npriority: 100\nsubunit: !binary |\n  w5ZyZQ==\n\nsubunit_to_unit: \"100\"\nsymbol: kr\n"

I have found two solutions to this, the first is to add serialize :currency to my model. The other way is to add a currency_iso_code method to the Money class and use that as a mapping instead.

spatrik commented Apr 29, 2010

I think patch broke the composed_of example for using the money gem with rails.

In my model I have:

composed_of :price,
  :class_name => "Money",
  :mapping => [%w(cents cents), %w(currency currency)]

Saving a variant object works, but when I load it back from the database, I get a YAML string instead:

 v2.currency # => "--- !ruby/object:Money::Currency \nid: :sek\niso_code: SEK\nname: Swedish Krona\npriority: 100\nsubunit: !binary |\n  w5ZyZQ==\n\nsubunit_to_unit: \"100\"\nsymbol: kr\n"

I have found two solutions to this, the first is to add serialize :currency to my model. The other way is to add a currency_iso_code method to the Money class and use that as a mapping instead.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Apr 29, 2010

Member

In my Rails app I extended the Money class with two additional methods

def currency_as_string
  currency.to_s
end

def currency_as_string=(value)
  currency = Currency.wrap(value)
end

And here's my model definition

composed_of :order_price,
            :class_name   => "Money",
            :mapping      => [%w( order_price_cents cents ), %w( order_price_currency currency_as_string )]
Member

weppos commented Apr 29, 2010

In my Rails app I extended the Money class with two additional methods

def currency_as_string
  currency.to_s
end

def currency_as_string=(value)
  currency = Currency.wrap(value)
end

And here's my model definition

composed_of :order_price,
            :class_name   => "Money",
            :mapping      => [%w( order_price_cents cents ), %w( order_price_currency currency_as_string )]
@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 29, 2010

Member

can you submit a patch to the documentation with you methods?

Member

semmons99 commented Apr 29, 2010

can you submit a patch to the documentation with you methods?

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Apr 29, 2010

Member

I have a doubt about the usefulness of my patch. The two methods above are strictly focused on ActiveRecord composite feature.

Whilst the first might be partially useful for the Money library itself, the second one is technically wrong. In fact, it allows you to change the internal value for currency causing an inconsistent object status.
Ideally, if we want to implement both methods, currency_as_string= should probably be a wrapper for the existing exchange_to method.

What do you think?

Member

weppos commented Apr 29, 2010

I have a doubt about the usefulness of my patch. The two methods above are strictly focused on ActiveRecord composite feature.

Whilst the first might be partially useful for the Money library itself, the second one is technically wrong. In fact, it allows you to change the internal value for currency causing an inconsistent object status.
Ideally, if we want to implement both methods, currency_as_string= should probably be a wrapper for the existing exchange_to method.

What do you think?

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 29, 2010

Member

I'm having a hard time deciding what to do here. I don't want to put ActiveRecord specific code into the library, and I don't see the use of currency_as_string, and especially currency_as_string=, outside of this.

I'm not very familiar with ActiveRecord, but is there anyway to use the :converter option in composed_of to make this work? Perhaps cents and currency should be stored in separate columns in the db?

Member

semmons99 commented Apr 29, 2010

I'm having a hard time deciding what to do here. I don't want to put ActiveRecord specific code into the library, and I don't see the use of currency_as_string, and especially currency_as_string=, outside of this.

I'm not very familiar with ActiveRecord, but is there anyway to use the :converter option in composed_of to make this work? Perhaps cents and currency should be stored in separate columns in the db?

@spatrik

This comment has been minimized.

Show comment
Hide comment
@spatrik

spatrik Apr 29, 2010

The :converter option is only used when the assigned object is of another class than specified (Money in this case). It can be used to make variant.price = 100 be converted to Money.new(10000, 'SEK') for example, but if I send a Money object the converter is not run.

In my example above I have cents and currency in separate columns, what happens is that AR runs something like write_attribute(:currency, money_object.send(:currency)). In 2.2, currency would have been a string, which gets saved properly, but since it's an object in 2.3, AR magically serializes it to YAML before saving it.

spatrik commented Apr 29, 2010

The :converter option is only used when the assigned object is of another class than specified (Money in this case). It can be used to make variant.price = 100 be converted to Money.new(10000, 'SEK') for example, but if I send a Money object the converter is not run.

In my example above I have cents and currency in separate columns, what happens is that AR runs something like write_attribute(:currency, money_object.send(:currency)). In 2.2, currency would have been a string, which gets saved properly, but since it's an object in 2.3, AR magically serializes it to YAML before saving it.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos Apr 29, 2010

Member

:converter and :constructor have a different goal. When you create a composite column called my_column, ActiveRecord creates a virtual my_column attribute with its own getter and setter methods.

You use :converter and :constructor to instruct how to cast an external object to fit the composite rule.
For example

composed_of :order_price,
            :class_name   => "Money",
            :mapping      => [%w( order_price_cents cents ), %w( order_price_currency currency_as_string )],
            :constructor  => Proc.new { |cents, currency| (cents.blank? || currency.blank?) ? nil : Money.new(cents, currency) },
            :converter    => Proc.new { |value| value.respond_to?(:to_money) ? value.to_money : raise(ArgumentError, "Can't convert #{value.class} to Money") }

record = Record.new
record.order_price = "$40"
# here the converter is called, equal to
# record.order_price = "$40".to_money
record.order_price = Object.new
# here the converter is called again
# raise ArgumentError, "Can't convert Object to Money"

ActiveRecord expects the values in the mapping hash to be compatible. If you set

:mapping      => [%w( order_price_cents cents ), %w( order_price_currency currency )],

It won't work because it tries to serialize the currency object instead of calling the #to_s method on it (I still have to understand the reason behind this choice. AFAIK, it may be an ActiveRecord bug). At least this is the issue I experiences so far.

I agree, I wouldn't change the Money Gem to fit an ActiveRecord use case. This is exactly the reason why I haven't included the patch before.
At least, we can extend the documentation to include references to this ticket and a quick solution.

Member

weppos commented Apr 29, 2010

:converter and :constructor have a different goal. When you create a composite column called my_column, ActiveRecord creates a virtual my_column attribute with its own getter and setter methods.

You use :converter and :constructor to instruct how to cast an external object to fit the composite rule.
For example

composed_of :order_price,
            :class_name   => "Money",
            :mapping      => [%w( order_price_cents cents ), %w( order_price_currency currency_as_string )],
            :constructor  => Proc.new { |cents, currency| (cents.blank? || currency.blank?) ? nil : Money.new(cents, currency) },
            :converter    => Proc.new { |value| value.respond_to?(:to_money) ? value.to_money : raise(ArgumentError, "Can't convert #{value.class} to Money") }

record = Record.new
record.order_price = "$40"
# here the converter is called, equal to
# record.order_price = "$40".to_money
record.order_price = Object.new
# here the converter is called again
# raise ArgumentError, "Can't convert Object to Money"

ActiveRecord expects the values in the mapping hash to be compatible. If you set

:mapping      => [%w( order_price_cents cents ), %w( order_price_currency currency )],

It won't work because it tries to serialize the currency object instead of calling the #to_s method on it (I still have to understand the reason behind this choice. AFAIK, it may be an ActiveRecord bug). At least this is the issue I experiences so far.

I agree, I wouldn't change the Money Gem to fit an ActiveRecord use case. This is exactly the reason why I haven't included the patch before.
At least, we can extend the documentation to include references to this ticket and a quick solution.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Apr 29, 2010

Member

Go ahead and write up the documentation and I'll merge it into trunk and release it.

Member

semmons99 commented Apr 29, 2010

Go ahead and write up the documentation and I'll merge it into trunk and release it.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos May 7, 2010

Member

http://github.com/weppos/money/compare/3d48d49a94950dcb297d...0dd2f8e78032d0054ee1

I also committed 3 other changes. Feel free to review and merge them if you agree with the changes.

Member

weppos commented May 7, 2010

http://github.com/weppos/money/compare/3d48d49a94950dcb297d...0dd2f8e78032d0054ee1

I also committed 3 other changes. Feel free to review and merge them if you agree with the changes.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 7, 2010

Contributor

Looks good. I see a typo though:
"+ # Alternativly you can use the convinience methods like"

Should be "Alternatively you can use the convenience methods like"

I'm not so happy with the ActiveRecord breakage. API breakages should result in a major version bump (e.g. 3.0.0) but the issue has been discovered after the release so no use in crying over spilled milk.

The ActiveRecord instructions should definitely be updated. You've written a new "Money 2.3 and ActiveRecord" section, but why isn't it part of the "Ruby on Rails" section? The "Ruby on Rails" section still contains incorrect code.

Contributor

FooBarWidget commented May 7, 2010

Looks good. I see a typo though:
"+ # Alternativly you can use the convinience methods like"

Should be "Alternatively you can use the convenience methods like"

I'm not so happy with the ActiveRecord breakage. API breakages should result in a major version bump (e.g. 3.0.0) but the issue has been discovered after the release so no use in crying over spilled milk.

The ActiveRecord instructions should definitely be updated. You've written a new "Money 2.3 and ActiveRecord" section, but why isn't it part of the "Ruby on Rails" section? The "Ruby on Rails" section still contains incorrect code.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 May 7, 2010

Member

I was thinking perhaps we should bump the major version once we get everything sorted out.

Member

semmons99 commented May 7, 2010

I was thinking perhaps we should bump the major version once we get everything sorted out.

@weppos

This comment has been minimized.

Show comment
Hide comment
@weppos

weppos May 7, 2010

Member

@ FooBarWidget

Fixed the typo, and reorganized the Rails section
http://github.com/weppos/money/compare/3d48d49a94950dcb297d...e0c7273

Member

weppos commented May 7, 2010

@ FooBarWidget

Fixed the typo, and reorganized the Rails section
http://github.com/weppos/money/compare/3d48d49a94950dcb297d...e0c7273

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 7, 2010

Contributor

Yes agreed with the major version bump.

@weppos: looks good. You should document that a breakage occurred in 2.3.0 though, and that previous versions used #currency and didn't need the patch.

Contributor

FooBarWidget commented May 7, 2010

Yes agreed with the major version bump.

@weppos: looks good. You should document that a breakage occurred in 2.3.0 though, and that previous versions used #currency and didn't need the patch.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 7, 2010

Contributor

I just noticed that the Github link in README.rdoc doesn't work very well. It links to this issue but the browser doesn't scroll to the correct comment. Can you document the issue directly in the README so that readers don't have to wade through all of our comments in search for the right one?

Contributor

FooBarWidget commented May 7, 2010

I just noticed that the Github link in README.rdoc doesn't work very well. It links to this issue but the browser doesn't scroll to the correct comment. Can you document the issue directly in the README so that readers don't have to wade through all of our comments in search for the right one?

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 May 7, 2010

Member

I merged weppos' changes into trunk and bumped the version. Take a look at the documentation and make sure we don't need anymore changes, then I'll release the gem.

Member

semmons99 commented May 7, 2010

I merged weppos' changes into trunk and bumped the version. Take a look at the documentation and make sure we don't need anymore changes, then I'll release the gem.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 May 7, 2010

Member

I also created two pages on the wiki to detail information about this issue.

Member

semmons99 commented May 7, 2010

I also created two pages on the wiki to detail information about this issue.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 7, 2010

Contributor

Looks good.

Contributor

FooBarWidget commented May 7, 2010

Looks good.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 May 7, 2010

Member

The gem has been released and the documentation has been updated.

Member

semmons99 commented May 7, 2010

The gem has been released and the documentation has been updated.

@semmons99

This comment has been minimized.

Show comment
Hide comment
@semmons99

semmons99 Jun 21, 2010

Member

#currency_as_string and #currency_as_string= have been added to Money as of version 3.0.3

Member

semmons99 commented Jun 21, 2010

#currency_as_string and #currency_as_string= have been added to Money as of version 3.0.3

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment