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

include migration instructions in Either projection deprecation message #7121

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

martijnhoekstra
Copy link
Contributor

addresses #6682 (comment)

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 22, 2018
@SethTisue SethTisue changed the title include migration instructions in deprecation message include migration instructions in Either projection deprecation message Aug 23, 2018
@@ -168,7 +168,7 @@ sealed abstract class Either[+A, +B] extends Product with Serializable {
*
* Because `Either` is right-biased, this method is not normally needed.
*/
@deprecated("Either is now right-biased", "2.13.0")
@deprecated("Either is now right-biased, use methods directly on Either", "2.13.0")
Copy link
Member

Choose a reason for hiding this comment

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

There's no get method on Either, should one be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable.

@SethTisue can and should we still squeeze that in?

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 added it to the PR, but I have no strong feelings on the matter one way or the other.

Copy link
Member

@SethTisue SethTisue Aug 25, 2018

Choose a reason for hiding this comment

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

meh, I think I'd rather not add more unsafe, non-total methods unless it's really a clear win. admittedly there is precedent on Option, but I basically think nobody's asking for this and we're probably better off without it. just as we might arguably be better off without Option#get (which we're probably stuck with, because tradition)

on Either, .getOrElse(throw ...) is already available

Copy link
Member

Choose a reason for hiding this comment

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

Then we need a better deprecation message when calling .right.get, I guess people are likely to replace it with .toOption.get (which is why I'd rather avoid the indirection and have Either#get)

Copy link
Member

Choose a reason for hiding this comment

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

Actually .toOption.get isn't great because it won't preserve the value of Left in the exception shown (which could be critical for debugging)

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 pushed a new proposal to this branch: the deprecated message points to getOrElse, and get is removed.

@@ -656,7 +656,7 @@ object Either {
*
* @author <a href="mailto:research@workingmouse.com">Tony Morris</a>, Workingmouse
*/
@deprecated("Either is now right-biased", "2.13.0")
@deprecated("Either is now right-biased, calls to right should be removed, and the operation should be used on Either directly", "2.13.0")
Copy link
Member

Choose a reason for hiding this comment

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

Put backquotes around right

@lrytz
Copy link
Member

lrytz commented Aug 29, 2018

@martijnhoekstra could you squash the changes into 1?

@martijnhoekstra
Copy link
Contributor Author

@lrytz this is 1 commit now

@lrytz
Copy link
Member

lrytz commented Aug 29, 2018

1️⃣ 🏆

@adriaanm adriaanm merged commit a44dd81 into scala:2.13.x Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants