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

Port to Cats. #528

Merged
merged 3 commits into from Aug 28, 2017
Merged

Port to Cats. #528

merged 3 commits into from Aug 28, 2017

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Aug 12, 2017

This is done in a style that attempts to minimize the diff from the Scalaz version, for as long as a Scalaz branch is supported.

Newts doesn’t support 2.10 and neither Cats nor Newts support Scala Native.
@julien-truffaut
Copy link
Member

Thank you so much Greg, I will try to review it and check why tests are failing this weekend.

@sellout
Copy link
Contributor Author

sellout commented Aug 18, 2017

The test that’s failing here is Map.Each.get what you set, which seems to be simply reversing the List that is the set value somehow. And it’s only failing on the JVM (although locally it seems to be failing on only JS ¯\_(ツ)_/¯).

Also, Travis still seems to be running 2.10 and Scala Native jobs, even though I thought I disabled those in both .travis.yml and build.sbt.

@julien-truffaut
Copy link
Member

I added the following unit test:

  test("each getAll") {
    val x = Map(1 -> "abc", 2 -> "cde")
    val e = each[Map[Int, String], String]
    e.modify(_.reverse)(x) should be (Map(1 -> "cba", 2 -> "edc"))
    e.getAll(x) should be (List("abc", "cde"))
    e.getAll(e.modify(_.reverse)(x)) should be (List("cba", "edc"))
  }

and I get:

[info] - each getAll *** FAILED ***
[info]   List("edc", "cba") was not equal to List("cba", "edc") (MapSpec.scala:18)

I suppose it comes from the Map Traverse instance in cats, it does not require the key to be ordered like scalaz does.

@julien-truffaut
Copy link
Member

I reported the issue in cats: typelevel/cats#1831

@tpolecat
Copy link
Contributor

So, I think the problem is that your test is assuming something that isn't necessarily true. The equivalence relation Eq isn't strong enough to prove congruence; a === b doesn't imply that f(a) === f(b) which is kind of at the root of this problem; sets a and b may be === but a.toList =!= b.toList, which is irritating but legal. So maybe all that needs to happen is to weaken the test expectations?

@julien-truffaut julien-truffaut changed the base branch from master to cats August 28, 2017 17:12
@julien-truffaut
Copy link
Member

I changed the target branch to cats as I prefer to make the first few releases with cats dependency using a different artifact (e.g. monocle-core-cats).

Thank you very much Greg, it is really an amazing work.

@julien-truffaut julien-truffaut merged commit 8344316 into optics-dev:cats Aug 28, 2017
@sellout sellout deleted the shims branch December 14, 2022 15:40
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

3 participants