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

Fixes type params for OptionT.getOrElse(F) #2119

Merged
merged 3 commits into from
Jan 22, 2018

Conversation

fcanedo
Copy link
Contributor

@fcanedo fcanedo commented Dec 18, 2017

Adds a type parameter to getOrElse and getOrElseF to conform to
Option.getOrElse, which allows it to be called without needing a type
hint:

OptionT(Future {
  Option(Right(1))
}).getOrElse(Left("error"))

Instead of:

OptionT(Future {
  Option(Right(1): Either[String, Int])
}).getOrElse(Left("error"))

Adds a type parameter to getOrElse and getOrElseF to conform to
Option.getOrElse, which allows it to be called without needing a type
hint:

OptionT(Future {
  Option(Right(1))
}).getOrElse(Left("error"))

Instead of:

OptionT(Future {
  Option(Right(1): Either[String, Int])
}).getOrElse(Left("error"))
@codecov-io
Copy link

codecov-io commented Dec 19, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2119      +/-   ##
==========================================
+ Coverage   94.63%   94.67%   +0.04%     
==========================================
  Files         325      328       +3     
  Lines        5514     5538      +24     
  Branches      221      199      -22     
==========================================
+ Hits         5218     5243      +25     
+ Misses        296      295       -1
Impacted Files Coverage Δ
core/src/main/scala/cats/data/OptionT.scala 97.46% <ø> (ø) ⬆️
core/src/main/scala/cats/Invariant.scala 90.47% <0%> (-4.77%) ⬇️
core/src/main/scala/cats/data/IndexedStateT.scala 89.24% <0%> (-0.12%) ⬇️
core/src/main/scala/cats/Semigroupal.scala 100% <0%> (ø) ⬆️
...e/src/main/scala/cats/instances/partialOrder.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/ApplicativeError.scala 88.23% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/invariant.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Const.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/ordering.scala 100% <0%> (ø) ⬆️
...ore/src/main/scala/cats/InvariantSemigroupal.scala 80% <0%> (ø)
... and 13 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 02b819c...cb2aba4. Read the comment docs.

ceedubs
ceedubs previously approved these changes Dec 19, 2017
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Thanks @fcanedo!

There is a downside to this, which is that if you pass in a value of the wrong type you'll get back something not very useful like OptionT[Future, Any] instead of a compile error.

However, this lines up with the getOrElse definitions on Validated, EitherT, and as you mentioned, Option. 👍 for consistency.

@ceedubs
Copy link
Contributor

ceedubs commented Dec 19, 2017

It might be nice to add a test for this, since it's functionality that we expect to work.

@kailuowang
Copy link
Contributor

Thanks! A doctest would be nice.

@fcanedo
Copy link
Contributor Author

fcanedo commented Dec 21, 2017

I added some unit tests, but what’s a “doctest”?

@kailuowang
Copy link
Contributor

kailuowang commented Dec 21, 2017

Doctest is really cool! You write code and expected result in Scaladoc, and it runs as a test. You can find plenty of examples here
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Apply.scala
To run them, you run sbt coreJVM/test

@fcanedo
Copy link
Contributor Author

fcanedo commented Dec 22, 2017

I can’t reproduce the errors that CI finds locally; any tips?

@kailuowang
Copy link
Contributor

It only happens on scala 2.10 and 2.11. In sbt you can type ++2.11.11 to test in scala 2.11. You might need to use "error".asLeft[Int] to help scala compiler prior to 2.12.

@fcanedo
Copy link
Contributor Author

fcanedo commented Jan 13, 2018

@kailuowang, your fix seems to solve the problem. Also, I confirmed that the tests fail to compile if I revert the fix. I think this is done.

What needs to happen to get this merged?

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Thanks @fcanedo!

@ceedubs ceedubs merged commit 32bcfc5 into typelevel:master Jan 22, 2018
@kailuowang kailuowang added this to the 1.1 milestone Jan 22, 2018
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