Skip to content

Allow subclassing money - #544

Merged
semmons99 merged 3 commits into
RubyMoney:masterfrom
nilbus:allow-subclassing-money
Jul 27, 2015
Merged

semmons99 merged 3 commits into
RubyMoney:masterfrom
nilbus:allow-subclassing-money

Conversation

@nilbus

@nilbus nilbus commented Jul 27, 2015

Copy link
Copy Markdown
Member

I can see from the spec previously included that it was desired that subclassing would work, but the implementation was incomplete.

Scenario:

class SpecialMoney < Money
  # ...
end

SpecialMoney.new(val1) + SpecialMoney.new(val2)
# => returns Money instead of SpecialMoney, thus losing the
# SpecialMoney extensions.

All specs remain green, and it works as expected. New specs have been added to attempt to prevent regression.

nilbus added 3 commits July 27, 2015 15:00
Scenario:

    class SpecialMoney < Money
      # ...
    end

    SpecialMoney.new(val1) + SpecialMoney.new(val2)
    # => returns Money instead of SpecialMoney, thus losing the
    # SpecialMoney extensions.
Currency#inspect is also defined but already does this.
Method calls on a subclass of Money should never return a Money object.
@joseph-ravenwolfe

Copy link
Copy Markdown

👍

@semmons99

Copy link
Copy Markdown
Member

nice!

semmons99 added a commit that referenced this pull request Jul 27, 2015
@semmons99
semmons99 merged commit 6655a38 into RubyMoney:master Jul 27, 2015
@nilbus

nilbus commented Jul 27, 2015

Copy link
Copy Markdown
Member Author

Thanks for the quick response! If you could release this quickly, that would be helpful to myself and @josephjaber. I believe this would qualify as a patch release (6.6.1), as it seems like a fix to something that was already intended. What do you think?

@semmons99

Copy link
Copy Markdown
Member

6.6.1 released

@nilbus

nilbus commented Jul 27, 2015

Copy link
Copy Markdown
Member Author

Thanks Shane!​

@tsyber1an

Copy link
Copy Markdown
Member

nice patch

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