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

Either .left, .right syntax #1454

Merged
merged 1 commit into from
Nov 7, 2016
Merged

Conversation

edmundnoble
Copy link
Contributor

No description provided.

@adelbertc
Copy link
Contributor

👍

@codecov-io
Copy link

codecov-io commented Nov 6, 2016

Current coverage is 92.20% (diff: 100%)

Merging #1454 into master will increase coverage by <.01%

@@             master      #1454   diff @@
==========================================
  Files           242        242          
  Lines          3615       3618     +3   
  Methods        3543       3550     +7   
  Messages          0          0          
  Branches         72         68     -4   
==========================================
+ Hits           3333       3336     +3   
  Misses          282        282          
  Partials          0          0          

Powered by Codecov. Last update 2fd55c5...2538b0e

@johnynek
Copy link
Contributor

johnynek commented Nov 6, 2016

Seems fine, but I'm slightly concerned that Either.right and .left which exist and are a bit different from this. I wonder if more verbose, yet clearer might be better. .asLeft or .toLeft?

@edmundnoble
Copy link
Contributor Author

@johnynek I can't see how Either.right and .left aren't the exact same as this, the same way Xor.right and .left were the same as their enriched syntax equivalents.

@johnynek
Copy link
Contributor

johnynek commented Nov 6, 2016

Either instances already have right/left methods. The object does not. So,
if we do: x.right.right the first and second mean different things. Also we
can't nest an either inside an either using .right since the enrichment
won't be triggered on the second call.

To me, this is less clear than it needs to be. .toRight or .toLeft would
resolve the ambiguity I think.
On Sat, Nov 5, 2016 at 18:42 Edmund Noble [email protected] wrote:

@johnynek https://github.com/johnynek I can't see how Either.right and
.left aren't the exact same as this, the same way Xor.right and .left
were the same as their enriched syntax equivalents.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1454 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJdkbM2jCech1LhinBljfA3EArB5fAks5q7VrIgaJpZM4KqdKE
.

@@ -306,6 +308,14 @@ final class RightOps[A, B](val right: Right[A, B]) extends AnyVal {
def leftCast[C]: Either[C, B] = right.asInstanceOf[Either[C, B]]
}

final class EitherIdOps[A](val obj: A) extends AnyVal {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this private val obj: A , otherwise everybody will have an obj that point to itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it to all the ops classes in here, for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kailuowang I checked and it looks like we don't use private vals in constructors for ops classes elsewhere in cats. Should I perhaps keep it not private for now for consistency, then submit a PR making all of them private?

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is the only one that applies to all types. So I would favor a bit inconsistency over having a surprising obj on all types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't the only one that applies to all types, see OptionIdOps. Everything has an a member already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, don't know how I missed that. I will submit a PR to fix OptionIdOps as soon as I get back to my laptop. Thanks for pointing it out. I still don't think being consistent with previous mistakes provides much value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You will also have to fix ApplicativeIdOps, ValidatedIdSyntax, and WriterIdSyntax. I don't think e.g. attaching an oa member to every Option[A] is particularly helpful either; are you sure we shouldn't make them all private?

Copy link
Contributor Author

@edmundnoble edmundnoble Nov 6, 2016

Choose a reason for hiding this comment

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

@kailuowang I have a PR (#1456) making the ops classes consistent in this regard.
Edit: You cannot make the val param of a value class private in 2.10. We're going to have to keep them all public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@edmundnoble
Copy link
Contributor Author

edmundnoble commented Nov 6, 2016

@johnynek Sorry for the miscommunication, I was referring to the Either.right and .left methods we enriched on to the Either companion object and not the projection methods defined on the values themselves. From that angle, I can certainly see where you're coming from and think asLeft and asRight could work.

Edit: The reason I favor asLeft and asRight over toLeft and toRight is that those two are already supplied on scala.Option to work with Either, and they do something different.

.left, .right => .asLeft, .asRight
@johnynek
Copy link
Contributor

johnynek commented Nov 6, 2016

👍 Looks good to me!

@kailuowang
Copy link
Contributor

👍
Thanks very much!

@kailuowang kailuowang merged commit 9af39fb into typelevel:master Nov 7, 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.

5 participants