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

standardise on liftF and add liftK to transformers #2033

Merged
merged 10 commits into from
Nov 27, 2017

Conversation

SystemFw
Copy link
Contributor

This PR deprecates lift on all transformers in favour of liftF, and adds a FunctionK version named liftK (for use with mapK).

I'm leaving a possible scalafix to a separate PR.

def apply[A](fa: F[A]): IndexedReaderWriterStateT[F, E, L, S, S, A] = IndexedReaderWriterStateT.liftF(fa)
}

@deprecated("Use liftF instead", "1.0.0-RC2")
Copy link
Contributor

Choose a reason for hiding this comment

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

There probably won't be a rc2, the next one is 1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, wasn't quite sure. I'll fix tomorrow

@kailuowang kailuowang added this to the 1.0.0 milestone Nov 21, 2017
@SystemFw
Copy link
Contributor Author

Fixed the RC deprecation warning. I'm adding the scalafix now (changed my mind, I'll do it in this PR)

@kailuowang
Copy link
Contributor

also need some mima exclusion

�[0m[�[31merror�[0m] �[0m filter with: ProblemFilters.excludeReversedMissingMethodProblem�[0m
�[0m[�[31merror�[0m] �[0m * method liftK(cats.Applicative,cats.kernel.Monoid)cats.arrow.FunctionK in trait cats.data.CommonIRWSTConstructors is present only in current version�[0m
�[0m[�[31merror�[0m] �[0m filter with: ProblemFilters.excludeReversedMissingMethodProblem�[0m
�[0m[�[31merror�[0m] �[0m * method liftF(java.lang.Object)cats.data.Kleisli in trait cats.data.KleisliFunctions is present only in current version�[0m
�[0m[�[31merror�[0m] �[0m filter with: ProblemFilters.excludeReversedMissingMethodProblem�[0m
�[0m[�[31merror�[0m] �[0m * method liftK()cats.arrow.FunctionK in trait cats.data.KleisliFunctions is present only in current version�[0m
�[0m[�[31merror�[0m] �[0m filter with: ProblemFilters.excludeReversedMissingMethodProblem�[0m
�[0m[�[31merror�[0m] �[0m * method liftF(java.lang.Object,cats.Applicative)cats.data.IndexedStateT in trait cats.data.CommonStateTConstructors is present only in current version�[0m
�[0m[�[31merror�[0m] �[0m filter with: ProblemFilters.excludeReversedMissingMethodProblem�[0m
�[0m[�[31merror�[0m] �[0m * method liftK(cats.Applicative)cats.arrow.FunctionK in trait cats.data.CommonStateTConstructors is present only in current version�[0m
�[0m[�[31merror�[0m] �[0m filter with: ProblemFilters.excludeReversedMissingMethodProblem�[0m

@SystemFw
Copy link
Contributor Author

@kailuowang Any quick reference to how you exclude something from MiMa or should I RTFM? :)

@LukaJCB
Copy link
Member

LukaJCB commented Nov 21, 2017

@SystemFw You can add them right here: https://github.com/typelevel/cats/blob/master/build.sbt#L212 :)

@SystemFw
Copy link
Contributor Author

Thanks a lot! @LukaJCB

@aeons
Copy link
Member

aeons commented Nov 22, 2017

This is great. I've had to write the OptionT variant several times lately.

Is there a reason you're not using the kind-projector syntax?

def liftK[F[_]](implicit F: Functor[F]): F ~> OptionT[F, ?] = 
  λ[F ~> OptionT[F, ?]](OptionT.liftF(_))

(or Lambda instead of λ)

@SystemFw
Copy link
Contributor Author

@aeons Yes, http4s was the initial use case :)

There's no reason for not using it, I might change it as I fix the other things

@SystemFw
Copy link
Contributor Author

I think I'm mostly done, except my scalafix test doesn't compile ( it's telling me that RWST is not a member of cats.data ).
This happens because the scalafix is running on 0.9.0, and RWST was introduced later. How should I proceed?

P.S. the error in the travis seems spurious

@kailuowang
Copy link
Contributor

the migration is for migrating from 0.9. ideally we need a new set of migration that migrates from 1.0 RC1, although I am not sure a simple rename in RWST justify that. I propose we just remove RWST from the test.

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #2033 into master will decrease coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2033      +/-   ##
==========================================
- Coverage   95.15%   94.98%   -0.18%     
==========================================
  Files         305      311       +6     
  Lines        5184     5267      +83     
  Branches      125      122       -3     
==========================================
+ Hits         4933     5003      +70     
- Misses        251      264      +13
Impacted Files Coverage Δ
core/src/main/scala/cats/data/IndexedStateT.scala 98.8% <100%> (-1.2%) ⬇️
core/src/main/scala/cats/data/EitherT.scala 97.6% <100%> (+0.01%) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 97.59% <100%> (-1.18%) ⬇️
core/src/main/scala/cats/data/WriterT.scala 93.75% <100%> (-0.94%) ⬇️
core/src/main/scala/cats/data/OptionT.scala 100% <100%> (ø) ⬆️
...in/scala/cats/data/IndexedReaderWriterStateT.scala 99.06% <100%> (-0.94%) ⬇️
core/src/main/scala/cats/Traverse.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/Foldable.scala 100% <0%> (ø) ⬆️
...ain/scala/cats/laws/discipline/TraverseTests.scala 100% <0%> (ø) ⬆️
...a/cats/laws/discipline/NonEmptyTraverseTests.scala 100% <0%> (ø) ⬆️
... and 10 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 8cff547...e68cade. Read the comment docs.

@SystemFw
Copy link
Contributor Author

SystemFw commented Nov 23, 2017

@kailuowang I think the errors are spurious and I should be done. The scalafix has fixes for both 0.9 and 1.0RC, the RC ones are not tested as discussed above

@SystemFw
Copy link
Contributor Author

Some of the builds fail with

Could not find any member to link for "mapK"

should I address this?

@LukaJCB
Copy link
Member

LukaJCB commented Nov 24, 2017

That seems odd, seems like an error during scaladoc compilation. Maybe someone else has an idea?

@SystemFw
Copy link
Contributor Author

SystemFw commented Nov 24, 2017

Yeah, I reference [[mapK]] in a scaladoc, might be a pretty stupid error ? . Can't really test locally since MiMa fails before on my machine ("versions not set" so it's my setup, it succeeds on Travis)

@LukaJCB
Copy link
Member

LukaJCB commented Nov 24, 2017

I think it might be because mapK is on the instance while liftF is on the companion object, so they need to be referred to differently. Not sure how exactly though.

@SystemFw
Copy link
Contributor Author

I think we agree on what the problem is then :)
I also think we agree in not knowing what the solution is :P
I'll investigate (this sort of search and replace PRs always end up taking ages for me)

@kailuowang
Copy link
Contributor

Linking reference in scaladoc has always been problematic, the solutions people have been doing in the past, and I propose doing here, is to just remove the link.

@SystemFw
Copy link
Contributor Author

SystemFw commented Nov 24, 2017

so just mapK rather than [[mapK]] ? Sounds good to me :)

kailuowang
kailuowang previously approved these changes Nov 25, 2017
@SystemFw
Copy link
Contributor Author

I need to fix the doctest for kleisli. It's the only bit of codecov that actually spotted a problem. Doing so right now

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.

Thanks!

@frroliveira
Copy link
Contributor

If you're still interested in getting the scaladoc references working, you can prefix mapK with the class name, e.g. EitherT.mapK. That's what I used in IorT.liftK

@kailuowang
Copy link
Contributor

Close and reopen to trigger build

@kailuowang kailuowang closed this Nov 25, 2017
@kailuowang kailuowang reopened this Nov 25, 2017
@LukaJCB LukaJCB mentioned this pull request Nov 26, 2017
3 tasks
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.

6 participants