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.

Sign up for GitHub

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

Jump to bottom

Either .left, .right syntax #1454

Merged
merged 1 commit into from
Nov 7, 2016
Merged

Either .left, .right syntax #1454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions core/src/main/scala/cats/syntax/either.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ trait EitherSyntax {
implicit def catsSyntaxLeft[A, B](left: Left[A, B]): LeftOps[A, B] = new LeftOps(left)

implicit def catsSyntaxRight[A, B](right: Right[A, B]): RightOps[A, B] = new RightOps(right)

implicit def catsSyntaxEitherId[A](a: A): EitherIdOps[A] = new EitherIdOps(a)
}

final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
Expand Down Expand Up @@ -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 •
edited
Loading

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!

/** Wrap a value in `Left`. */
def asLeft[B]: Either[A, B] = Left(obj)

/** Wrap a value in `Right`. */
def asRight[B]: Either[B, A] = Right(obj)
}

/** Convenience methods to use `Either` syntax inside `Either` syntax definitions. */
private[cats] object EitherUtil {
def leftCast[A, B, C](right: Right[A, B]): Either[C, B] =
Expand Down
7 changes: 7 additions & 0 deletions tests/src/test/scala/cats/tests/EitherTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ class EitherTests extends CatsSuite {
}
}

test("Left/Right id syntax") {
forAll { (e: Int) =>
assert(Left[Int, String](e) === e.asLeft[String])
assert(Right[String, Int](e) === e.asRight[String])
}
}

test("implicit instances resolve specifically") {
val eq = catsStdEqForEither[Int, String]
assert(!eq.isInstanceOf[PartialOrder[_]])
Expand Down