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

Add Chain equals & hashCode methods (via Chain.iterator?) #2540

Closed
seigert opened this issue Sep 28, 2018 · 7 comments
Closed

Add Chain equals & hashCode methods (via Chain.iterator?) #2540

seigert opened this issue Sep 28, 2018 · 7 comments

Comments

@seigert
Copy link

seigert commented Sep 28, 2018

Currently Chain[A] only defines === operator and therefore different Chain instances with "same" contents are not .equals to each other:

import cats.data.Chain
import cats.implicits._

(Chain.one(1) |+| Chain.one(2) |+| Chain.one(3)) == Chain.fromSeq(List(1, 2, 3)) // false
(Chain.one(1) |+| Chain.one(2) |+| Chain.one(3)) === Chain.fromSeq(List(1, 2, 3)) // true

Not only this leads to confusion, but also could lead to bugs if Chain[A] used in Set or as Map keys, or in Eq.fromUniversalEquals.

@calvinbrown085
Copy link
Contributor

So, for this do we want a == method on Chain ?

@kubukoz
Copy link
Member

kubukoz commented Oct 14, 2018

You can't define ==, but it's desugared to equals :)

@ceedubs
Copy link
Contributor

ceedubs commented Oct 14, 2018

I think that it should be pretty straightforward to implement equals in terms of === and Eq.fromUniversalEquals; and hashCode in terms of Chain.iterator, StaticMethods.orderedHash, and Hash.fromUniversalHashCode.

@simonyun
Copy link

simonyun commented Jan 8, 2019

🙋🏻‍♂️

Chiming in with a vote — this was causing my ScalaTest assertResult check to fail in the context of ValidatedNec.

Welcome to Scala 2.12.7 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_192).
Type in expressions for evaluation. Or try :help.

scala> import cats.data.ValidatedNec
import cats.data.ValidatedNec

scala> type ExampleValidatedNec = ValidatedNec[String, Int]
defined type alias ExampleValidatedNec

scala> import cats.data._
import cats.data._

scala> import cats.implicits._
import cats.implicits._

scala> val validatedInt1 = Validated.Invalid("foo").toValidatedNec
validatedInt1: cats.data.ValidatedNec[String,Nothing] = Invalid(Chain(foo))

scala> val validatedInt2 = Validated.Invalid("bar").toValidatedNec
validatedInt2: cats.data.ValidatedNec[String,Nothing] = Invalid(Chain(bar))

scala> case class TwoInts(int1: Int, int2: Int)
defined class TwoInts

scala> val exampleValidatedNec2 = (validatedInt1, validatedInt2).mapN(TwoInts.apply)
exampleValidatedNec2: cats.data.ValidatedNec[String,TwoInts] = Invalid(Chain(foo, bar))

scala> val expectedNecOfTwo = NonEmptyChain("foo", "bar")
expectedNecOfTwo: cats.data.NonEmptyChain[String] = Chain(foo, bar)

scala> exampleValidatedNec2.toEither.left.get == expectedNecOfTwo // expect true
res0: Boolean = true

scala> case class ThreeInts(int1: Int, int2: Int, int3: Int)
defined class ThreeInts

scala> val validatedInt3 = Validated.Invalid("baz").toValidatedNec
validatedInt3: cats.data.ValidatedNec[String,Nothing] = Invalid(Chain(baz))

scala> val exampleValidatedNec3 = (validatedInt1, validatedInt2, validatedInt3).mapN(ThreeInts.apply)
exampleValidatedNec3: cats.data.ValidatedNec[String,ThreeInts] = Invalid(Chain(foo, bar, baz))

scala> val expectedNecOfThree = NonEmptyChain("foo", "bar", "baz")
expectedNecOfThree: cats.data.NonEmptyChain[String] = Chain(foo, bar, baz)

scala> exampleValidatedNec3.toEither.left.get == expectedNecOfThree // expect true
res1: Boolean = false

scala> exampleValidatedNec3.toEither.left.get === expectedNecOfThree // expect true
res2: Boolean = true

So it looks like somehow the equality check works for two or fewer elements, but not for three. Obviously I will change my test to use ===, but it'd be great to have Object.equals overridden and functioning here.

@kailuowang
Copy link
Contributor

kailuowang commented Jan 8, 2019

👍 on @ceedubs' suggestion.

@ceedubs
Copy link
Contributor

ceedubs commented Jan 8, 2019

I'll give this a try.

ceedubs added a commit to ceedubs/cats that referenced this issue Jan 8, 2019
@ceedubs
Copy link
Contributor

ceedubs commented Jan 8, 2019

#2690 submitted for this

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

No branches or pull requests

6 participants