Add scalafix for contramap #1937

LukaJCB · 2017-09-29T12:58:11Z

This PR provides an automatic fix for #1850, which should provide a seamless upgrading experience :)

kailuowang · 2017-09-29T13:04:41Z

scalafix/rules/src/main/scala/fix/Cats_v1_0_0.scala

+case class ContraMapToLMap(index: SemanticdbIndex)
+  extends SemanticRule(index, "UseLMapInsteadOfContraMap") {
+
+  //val contraMap = Symbol("_root_.cats.functor.Contravariant.Ops.contramap")


kailuowang · 2017-09-29T13:06:13Z

scalafix/rules/src/main/scala/fix/Cats_v1_0_0.scala

+    ctx.tree.collect {
+      case Term.Apply(fun, _) =>
+        if (contraMatcher.matches(fun) &&
+          fun.children.headOption.flatMap(index.denotation).exists{ x => println(x.name == unApplyName); x.name == unApplyName }) {


Is this println intentionally left?

This denotation trick is nice

kailuowang · 2017-09-29T13:08:18Z

This is awesome! @LukaJCB . I would say this fixes #1850

LukaJCB · 2017-09-29T13:12:00Z

Btw, it might be good to test these with CI in the future :)

kailuowang · 2017-09-29T13:13:42Z

It's not? How did you test it locally? Is it hard to add it to travis?

LukaJCB · 2017-09-29T13:16:45Z

It's a different sbt project, let me see what I can do :)

codecov-io · 2017-09-29T13:22:44Z

Codecov Report

Merging #1937 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1937      +/-   ##
=========================================
+ Coverage   95.57%   95.6%   +0.02%     
=========================================
  Files         248     252       +4     
  Lines        4454    4570     +116     
  Branches      126     120       -6     
=========================================
+ Hits         4257    4369     +112     
- Misses        197     201       +4

Impacted Files	Coverage Δ
kernel/src/main/scala/cats/kernel/Order.scala	`81.08% <0%> (-5.41%)`	⬇️
laws/src/main/scala/cats/laws/discipline/Eq.scala	`90.9% <0%> (-2.2%)`	⬇️
core/src/main/scala/cats/InvariantMonoidal.scala	`0% <0%> (ø)`	⬆️
...nel/src/main/scala/cats/kernel/instances/map.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/Show.scala	`100% <0%> (ø)`	⬆️
...in/scala/cats/laws/discipline/BifunctorTests.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/Functor.scala	`100% <0%> (ø)`	⬆️
.../src/main/scala/cats/kernel/instances/option.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/Composed.scala	`95.83% <0%> (ø)`	⬆️
.../src/main/scala/cats/kernel/instances/vector.scala	`100% <0%> (ø)`	⬆️
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e33b29...eebcf02. Read the comment docs.

kailuowang · 2017-09-29T13:28:44Z

.travis.yml

@@ -30,6 +30,11 @@ matrix:
 before_install:
 - export PATH=${PATH}:./vendor/bundle

+before_script:


adding this to https://github.com/typelevel/cats/blob/master/scripts/travis-publish.sh might be better.

Agreed. Just want to test for now :)

kailuowang · 2017-10-04T11:18:51Z

Another PR added some rules that only applies to the yet to release RC1. So we need to let the output depends on a snapshot of head, need to publish the head snapshot locally first at CI.

LukaJCB · 2017-10-04T12:11:42Z

Seems like a good idea 👍 I'm not sure how to go forward with that though, you have any pointers?

kailuowang · 2017-10-04T14:19:39Z

@LukaJCB, change here https://github.com/typelevel/cats/blob/master/scalafix/build.sbt#L17
to point to 1.0.0-SNAPSHOT instead of 1.0.0-MF.
Then in the travis build script add another step "sbt publishLocal" so it's

sbt ;coreJVM/publishLocal;freeJVM/publishLocal
cd scalafix
sbt tests/test
cd ..

gabro

left some minor comments on Scalafix api usage, I also suspect the denotation lookup could be easier (still not sure).

EDIT: as discussed on gitter, this is a simpler alternative for matching contramap in the desired cases:

    ctx.tree.collect {
      case Term.Apply(Term.Select(f, contraMatcher(contramap)), _) if f.denotation.exists(_.name == unApplyName) =>
        ctx.replaceTree(contramap, "lmap")
    }.asPatch

gabro · 2017-10-05T14:25:57Z

scalafix/rules/src/main/scala/fix/Cats_v1_0_0.scala

+    val unApplyName = "catsUnapply2left"
+
+    ctx.tree.collect {
+      case Term.Apply(fun, _) =>


SymbolMatches have an unapply method, so you can use it like

case Term.Apply(contraMatcher(fun), _) =>

gabro · 2017-10-05T14:26:17Z

scalafix/rules/src/main/scala/fix/Cats_v1_0_0.scala

+      case Term.Apply(fun, _) =>
+        if (contraMatcher.matches(fun) &&
+          fun.children.headOption.flatMap(index.denotation).exists(_.name == unApplyName )) {
+          fun.children.find(contraMatcher.matches).map(tree => ctx.replaceTree(tree, "lmap")).getOrElse(Patch.empty)


.getOrElse(Patch.empty) can become .asPatch

gabro · 2017-10-05T14:26:45Z

scalafix/rules/src/main/scala/fix/Cats_v1_0_0.scala

+        } else {
+          Patch.empty
+        }
+      case _ => Patch.empty


no need for this case, since this is a collect

LukaJCB · 2017-10-05T16:09:56Z

I simplified a lot thanks to @gabro! 🎉

edmundnoble

Love it

edmundnoble · 2017-10-10T03:16:08Z

scripts/travis-publish.sh

@@ -34,6 +35,11 @@ free_js="$sbt_cmd validateFreeJS"
 js="$core_js && $free_js && $kernel_js"
 jvm="$sbt_cmd coverage validateJVM coverageReport && codecov"

+sbt ;coreJVM/publishLocal;freeJVM/publishLocal
+cd scalafix
+sbt tests/test


I expected this to add a lot to build time, but apparently not!

LukaJCB · 2017-10-10T07:36:40Z

Merging with two sign offs :)

Add scalafix for contramap

a148cf1

stew added the in progress label Sep 29, 2017

kailuowang reviewed Sep 29, 2017

View reviewed changes

Remove redundancies

50631c1

kailuowang approved these changes Sep 29, 2017

View reviewed changes

kailuowang reviewed Sep 29, 2017

View reviewed changes

Try to build scalafix with travis

f6c024f

LukaJCB force-pushed the scalafix-for-contramap branch from 3ac35a6 to f6c024f Compare September 29, 2017 13:57

LukaJCB removed the in progress label Sep 30, 2017

Working travis tests

08c85a7

LukaJCB force-pushed the scalafix-for-contramap branch from d30db27 to 08c85a7 Compare September 30, 2017 16:52

LukaJCB added this to the 1.0.0-RC1 milestone Oct 1, 2017

kailuowang previously approved these changes Oct 3, 2017

View reviewed changes

Publish snapshot before running scalafix

562bef9

LukaJCB dismissed kailuowang’s stale review via 562bef9 October 4, 2017 15:14

kailuowang previously approved these changes Oct 5, 2017

View reviewed changes

gabro reviewed Oct 5, 2017

View reviewed changes

Simplify denotation lookup

eebcf02

LukaJCB dismissed kailuowang’s stale review via eebcf02 October 5, 2017 16:11

kailuowang approved these changes Oct 5, 2017

View reviewed changes

LukaJCB mentioned this pull request Oct 8, 2017

Rename EitherT.liftT to EitherT.liftF #1947

Merged

edmundnoble approved these changes Oct 10, 2017

View reviewed changes

LukaJCB merged commit b3244a8 into typelevel:master Oct 10, 2017

kailuowang mentioned this pull request Oct 10, 2017

contramap on Function1 broke between 0.9 to 1.0.0-MF #1850

Closed

kailuowang modified the milestone: 1.0.0-RC1 Oct 13, 2017

kailuowang added the enhancement label Oct 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scalafix for contramap #1937

Add scalafix for contramap #1937

LukaJCB commented Sep 29, 2017

kailuowang Sep 29, 2017

kailuowang Sep 29, 2017

kailuowang Sep 29, 2017

kailuowang commented Sep 29, 2017

LukaJCB commented Sep 29, 2017

kailuowang commented Sep 29, 2017

LukaJCB commented Sep 29, 2017

codecov-io commented Sep 29, 2017 •

edited

Loading

kailuowang Sep 29, 2017

LukaJCB Sep 29, 2017

kailuowang commented Oct 4, 2017

LukaJCB commented Oct 4, 2017

kailuowang commented Oct 4, 2017

gabro left a comment •

edited

Loading

gabro Oct 5, 2017

gabro Oct 5, 2017

gabro Oct 5, 2017

LukaJCB commented Oct 5, 2017

edmundnoble left a comment

edmundnoble Oct 10, 2017

LukaJCB commented Oct 10, 2017

Add scalafix for contramap #1937

Add scalafix for contramap #1937

Conversation

LukaJCB commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Sep 29, 2017

LukaJCB commented Sep 29, 2017

kailuowang commented Sep 29, 2017

LukaJCB commented Sep 29, 2017

codecov-io commented Sep 29, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Oct 4, 2017

LukaJCB commented Oct 4, 2017

kailuowang commented Oct 4, 2017

gabro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Oct 5, 2017

edmundnoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Oct 10, 2017

codecov-io commented Sep 29, 2017 •

edited

Loading

gabro left a comment •

edited

Loading