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

improve interaction with subtyping in OptionOps#liftTo #2480

Merged
merged 4 commits into from
Sep 12, 2018
Merged

improve interaction with subtyping in OptionOps#liftTo #2480

merged 4 commits into from
Sep 12, 2018

Conversation

mberndt123
Copy link
Contributor

Currently OptionOps#liftTo interacts badly with subtyping. Something like this won't compile:

42.some.liftTo[IO](new Exception("foo"))

One needs to write this instead:

42.some.liftTo[IO](new Exception("foo"): Throwable)

This patch fixes this.

@codecov-io
Copy link

codecov-io commented Sep 7, 2018

Codecov Report

Merging #2480 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2480   +/-   ##
=======================================
  Coverage   95.38%   95.38%           
=======================================
  Files         357      357           
  Lines        6517     6517           
  Branches      279      282    +3     
=======================================
  Hits         6216     6216           
  Misses        301      301
Impacted Files Coverage Δ
core/src/main/scala/cats/ApplicativeError.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/option.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/either.scala 99.21% <100%> (ø) ⬆️

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 92236f8...53f7cbb. Read the comment docs.

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

This is nice. Would you add a doctest to verify the new ergonomics?

@kailuowang
Copy link
Contributor

kailuowang commented Sep 8, 2018

Also the liftTo on Either can be improved the same way right?

@kailuowang
Copy link
Contributor

Thanks! BTW, in case you haven't used doctest before, here is an example of PR adding doctests, it would nice to have test make sure that this new interaction works as intended.

@mberndt123
Copy link
Contributor Author

mberndt123 commented Sep 8, 2018

I added the same change to EitherOps#liftTo.

I also added doctests. The latter turned out to be a PITA. I added the doctests, and they compiled and ran. In order to see whether they actually exercise my new code, I changed the _ >: E in OptionOps.LiftToPartiallyApplied part back to E and expected them to fail. But they still ran just fine, and I couldn't figure out what the hell was going on… Until, after quite a bit of head scratching, I ran clean in sbt before coreJVM/test and, lo and behold, the tests started failing as expected! Apparently incremental compilation with sbt/zinc/scalac is still broken ☹️

Oh well, one more scala war story to tell.

LukaJCB
LukaJCB previously approved these changes Sep 8, 2018
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

kailuowang
kailuowang previously approved these changes Sep 8, 2018
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

This is nice. Thanks!

@mberndt123
Copy link
Contributor Author

mberndt123 commented Sep 8, 2018

I've simplified the tests a bit. The reason I used EitherT[Option, CharSequence, ?] instead of just Either[CharSequence, ?] in the test is that my test was passing when I expected it to fail. I suspected that Either's covariance might have something to do with it, so I changed the test to use EitherT, which is invariant. But the actual cause turned out to be the above-mentioned incremental compilation bug, so there's no reason to complicate things with EitherT.

@mberndt123 mberndt123 dismissed stale reviews from kailuowang and LukaJCB via 53f7cbb September 8, 2018 14:18
@mberndt123
Copy link
Contributor Author

@kailuowang @LukaJCB could you approve again?

@mberndt123
Copy link
Contributor Author

This PR was approved and all review comments were addressed, so to me it looks like it's ready to be merged. Is there anything else to do to get this merged?

@LukaJCB
Copy link
Member

LukaJCB commented Sep 12, 2018

Sorry for the delay, merging now :)

@LukaJCB LukaJCB merged commit fe1bd81 into typelevel:master Sep 12, 2018
@mberndt123
Copy link
Contributor Author

Cool, thanks!

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

Successfully merging this pull request may close these issues.

4 participants