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

Improve the type signature of orElse #575

Merged

Conversation

jedesah
Copy link
Contributor

@jedesah jedesah commented Oct 16, 2015

The whole point of orElse is to drop the left hand value of a Disjunction, no sense having the type that is dropped influence the resulting type.

Let me know if I am missing something! :-)

@codecov-io
Copy link

Current coverage is 75.70%

Merging #575 into master will decrease coverage by -0.01% as of f0ef537

@@            master   #575   diff @@
=====================================
  Files          157    157       
  Stmts         2166   2165     -1
  Branches        68     68       
  Methods          0      0       
=====================================
- Hit           1640   1639     -1
  Partial          0      0       
  Missed         526    526       

Review entire Coverage Diff as of f0ef537

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor

ceedubs commented Oct 16, 2015

👍 for the idea and type signature change.

If the original Xor is a Right, it would be nice to save an allocation by returning that same instance. I don't think fold will let you do that (at least not without an asInstanceOf), but I think if we replaced the implementation with a match we could. Should we give that a try?

The whole point of orElse is to drop the left hand value of a Disjunction, no sense having the type that is dropped influence the resulting type.
@jedesah jedesah force-pushed the topic/improve_signature_orElse_of_Xor branch from 798d4c1 to 68e4e49 Compare October 16, 2015 17:20
@jedesah
Copy link
Contributor Author

jedesah commented Oct 16, 2015

@ceedubs Sure!

@ceedubs
Copy link
Contributor

ceedubs commented Oct 16, 2015

👍 thanks!

fold(_ => fallback, _ => this)
def orElse[C, BB >: B](fallback: => C Xor BB): C Xor BB = this match {
case Xor.Left(_) => fallback
case r @ Xor.Right(_) => r
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a difference in

case r @ Xor.Right(_) => r

and

case r => r

? The former is probably nicer to read, but does it come at a cost? i don't actually know and have wondered about this.

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 would be surprised if it did not come at a cost, but who knows, maybe the scala pattern matcher is smarter than I give it credit for. Happy to change it to r => r.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will essentially correspond to an else case, so it seems fine to me.

@stew
Copy link
Contributor

stew commented Oct 16, 2015

👍

stew added a commit that referenced this pull request Oct 16, 2015
…f_Xor

Improve the type signature of orElse
@stew stew merged commit 42baab7 into typelevel:master Oct 16, 2015
ceedubs added a commit to ceedubs/cats that referenced this pull request Nov 7, 2015
This is the same as typelevel#575 but for Validated instead of Xor.
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