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

Put liftT on the companion object of XorT #1215

Merged
merged 5 commits into from
Aug 20, 2016

Conversation

zmccoy
Copy link
Member

@zmccoy zmccoy commented Jul 18, 2016

I found it useful to have liftT available to me for wrapping values in XorT. I couldn't figure out what test should be added in this case, but if anyone has any examples or suggestions I'd love to add one.

@codecov-io
Copy link

codecov-io commented Jul 18, 2016

Current coverage is 89.27% (diff: 100%)

Merging #1215 into master will increase coverage by 0.08%

@@             master      #1215   diff @@
==========================================
  Files           234        234          
  Lines          3144       3161    +17   
  Methods        3085       3104    +19   
  Messages          0          0          
  Branches         57         54     -3   
==========================================
+ Hits           2804       2822    +18   
+ Misses          340        339     -1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 885acf6...1e277cd

@ceedubs
Copy link
Contributor

ceedubs commented Jul 18, 2016

@zmccoy thanks! This looks good.

Only one minor suggestion: to be consistent with other functions, could you add this to the XorTFunctions trait that gets mixed into the XorT companion object instead of defining it directly on the companion object? I wouldn't really be opposed to us having all of the methods directly in the companion object instead of using the mix-in trait, but since it's there I'd rather be consistent.

@peterneyens
Copy link
Collaborator

I think XorT.liftT is analogous to the existing XorT.right, I am not sure if we need both ?

We also have the syntax rightXorT :

1.some.rightXorT[String]
// XorT[Option,String,Int] = XorT(Some(Right(1)))

XorT.right[Option, String, Int](1.some)
// XorT[Option,String,Int] = XorT(Some(Right(1)))

It is a good idea to use XorT.liftT / XorT.right in the TransLift instance of XorT.

@ceedubs
Copy link
Contributor

ceedubs commented Jul 18, 2016

@peterneyens ah good point. I had forgotten about that.

If a liftT method would be more discoverable for people, then I don't think I have any objection to adding it as an alias for right, but it also seems fine to me to either let people use the existing methods.

@zmccoy
Copy link
Member Author

zmccoy commented Jul 18, 2016

@peterneyens I somehow completely missed that when I skimmed the source!
@ceedubs I'm fine with either, just let me know what you all would prefer and I can get it up to par :)

@zmccoy
Copy link
Member Author

zmccoy commented Jul 18, 2016

I've put the function in the correct place, added it as an alias to XorT.right, and it's used in the Translift typeclass impl for XorT.

/**
* Alias for XorT.right
*/
final def liftT[F[_], A, B](fb: F[B])(implicit F: Functor[F]): XorT[F, A, B] = right(fb)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we even need Functor here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the implicit evidence for a Functor it won't compile since right needs one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. I made the mistake of believing that was just Xor.right Sorry for the noise.

Copy link
Member Author

Choose a reason for hiding this comment

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

All good, I'm just happy I could answer :)

@ceedubs
Copy link
Contributor

ceedubs commented Jul 19, 2016

Thanks @zmccoy!

Would you want to add an sbt-doctest example (for example like this)? I think these are useful for people looking at the scaladoc but also help us to see whether there are any inference issues that make methods like this cumbersome to use in practice.

/**
* Alias for [[XorT.right]]
*/
final def liftT[F[_], A, B](fb: F[B]): XorT[F, A, B] = right(fb)
Copy link
Contributor

Choose a reason for hiding this comment

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

This currently isn't compiling, because it needs an F[_]: Functor constraint here for the call to right.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my bad, I pushed up a bad commit with the scaladoc link in it. I ended up in a hurry and my laptop was dieing. I'll get that all fixed along with the sbt-doctest later this evening / tomorrow

@zmccoy
Copy link
Member Author

zmccoy commented Jul 21, 2016

@ceedubs I'm not very familiar with sbt-doctest but I think that's what you were looking for, but if it isn't I'd love to fix it up. Let me know if you need anything else.

* Alias for [[XorT.right]]
* {{{
* scala> import cats.data.XorT
* scala> import cats.instances.option._
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please use import cats.implicits._ here? See #1026 for justification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally, thanks for the link.

@ceedubs
Copy link
Contributor

ceedubs commented Jul 21, 2016

👍 thanks!

@zmccoy
Copy link
Member Author

zmccoy commented Aug 19, 2016

Hi @ceedubs is there anything else I need to do on this for review? :)

@kailuowang
Copy link
Contributor

kailuowang commented Aug 20, 2016

👍 Thanks very much!

@kailuowang kailuowang merged commit 61a2a16 into typelevel:master Aug 20, 2016
adelbertc added a commit to adelbertc/cats that referenced this pull request Aug 20, 2016
adelbertc added a commit to adelbertc/cats that referenced this pull request Aug 20, 2016
kailuowang added a commit that referenced this pull request Aug 20, 2016
travisbrown added a commit that referenced this pull request Aug 20, 2016
kcsongor pushed a commit to kcsongor/cats that referenced this pull request Aug 21, 2016
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.

6 participants