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

#323 adding hashcode to Money #351

Merged
merged 3 commits into from Sep 20, 2019
Merged

Conversation

Vrolijkx
Copy link

PR to solve bug #323.
This makes it possible to use Money and Price objects correctly in Sets/Maps. Or when these objects are nested in a case class

Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, it ooks good in general, some minor comments inline

@@ -14,6 +14,7 @@ import scala.util.{Failure, Success, Try}
import scala.language.implicitConversions
import scala.math.BigDecimal.RoundingMode
import scala.math.BigDecimal.RoundingMode.RoundingMode
import java.util.Objects
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is import is unused and it is breaking the build

Copy link
Author

Choose a reason for hiding this comment

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

I removed the java.util package from the hash function.

@@ -377,6 +384,9 @@ final class Money private (val amount: BigDecimal)(val currency: Currency)
* @return Money
*/
def mapAmount(f: BigDecimal => BigDecimal) = currency(f(amount))


Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove the extra lines?

Copy link
Author

Choose a reason for hiding this comment

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

I fixed the formatting back to the original(style).

Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

LGTM

@Vrolijkx
Copy link
Author

@cquiroz can somebody merge looks like I don't have the permission to do that.

@cquiroz
Copy link
Collaborator

cquiroz commented Sep 15, 2019

@Vrolijkx We've had the policy of having two people approve a PR, however if nobody approves in a few days I'll merge. I was hoping to use your PR to test the automated release but I think I won't be ready by then

it should "return consistent hashcode" in {
val someMoney = USD(2.1)

someMoney.hashCode() shouldBe someMoney.hashCode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Java requires equal hash codes for equal objects, so I suggest adding a test which also checks against another instance of USD(2.1)

@garyKeorkunian garyKeorkunian merged commit d23f53a into typelevel:master Sep 20, 2019
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.

None yet

4 participants