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.

Sign up for GitHub

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

Jump to bottom

Add Comparable to Spree::Money #1682

Merged
merged 1 commit into from
Jan 16, 2017
Merged

Add Comparable to Spree::Money #1682

merged 1 commit into from
Jan 16, 2017

Conversation

graygilmore
Copy link
Contributor

Fixes #1650

graygilmore
graygilmore commented Jan 12, 2017
View reviewed changes
core/spec/lib/spree/money_spec.rb
@@ -268,4 +268,32 @@
end
end
end

describe "<=>" do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about also adding tests for <, >, .between?, and .sort as those also get included with Comparable but I didn't want to get into the habit of testing another module. Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about testing that Comparable is included? This way, if we remove that module, we'll be sure that some test will fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to testing that Comparable is included 👍

No need to test a well tested module from Rubys standard lib

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, I'll include that.

kennyadsl
kennyadsl reviewed Jan 12, 2017
View reviewed changes
core/lib/spree/money.rb
"Can't compare #{self.currency} with #{other.currency}"
)
end
self.money <=> other.money
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure which one is better here but in the rest of the file to read attributes we used @money instead of self.money. Is there any reason to change this here? (I think it also applies to self.currency -> @currency). If they are equivalent we should probably be more consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@money would work as this is a attr_reader, won't work for @currency, though, as this is a delegate to money

👍 for being consistent on @money and currency usage.

🤔 thinking to enable Rubocops "redundant self" checks...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

core/spec/lib/spree/money_spec.rb
@@ -268,4 +268,32 @@
end
end
end

describe "<=>" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about testing that Comparable is included? This way, if we remove that module, we'll be sure that some test will fail.

tvdeyen
tvdeyen approved these changes Jan 12, 2017
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only little nits, nothing that would hold us back from merging

core/spec/lib/spree/money_spec.rb
@@ -268,4 +268,32 @@
end
end
end

describe "<=>" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to testing that Comparable is included 👍

No need to test a well tested module from Rubys standard lib

core/lib/spree/money.rb
"Can't compare #{self.currency} with #{other.currency}"
)
end
self.money <=> other.money
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@money would work as this is a attr_reader, won't work for @currency, though, as this is a delegate to money

👍 for being consistent on @money and currency usage.

🤔 thinking to enable Rubocops "redundant self" checks...

@graygilmore graygilmore force-pushed the spree-money/comparable branch from 238626b to 5ad8d6d Compare January 13, 2017 17:14
@graygilmore
Copy link
Contributor Author

@kennyadsl @tvdeyen fixed the @money and added a test that checks if Comparable is included. I went back and forth a few times on the value of adding that test.

jhawthorn
jhawthorn approved these changes Jan 16, 2017
View reviewed changes
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

This could use a changelog entry

@graygilmore
Add Comparable to Spree::Money
943b2c5 
This commit includes Comparable and implements `<=>`. Including
Comparable also gives us some basic comparison methods like `<`, `>`,
`.between?`, and `sort`.

We need to add an additional currency comparison because `::Money` will
automatically try to convert the second currency in order to be able to
compare it with the first. We don't want this to happen so we're going
to explicitly raise an error.
@graygilmore graygilmore force-pushed the spree-money/comparable branch from 5ad8d6d to 943b2c5 Compare January 16, 2017 19:48
@graygilmore
Copy link
Contributor Author

@jhawthorn changelog entry added.

@jhawthorn jhawthorn merged commit 2dcc0ed into solidusio:master Jan 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennyadsl kennyadsl kennyadsl left review comments

@jhawthorn jhawthorn jhawthorn approved these changes

@tvdeyen tvdeyen tvdeyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@graygilmore @tvdeyen @jhawthorn @kennyadsl