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

Coeval flatmap combinator #935

Merged
merged 3 commits into from Jul 18, 2019

Conversation

@valenterry
Copy link
Contributor

commented Jun 27, 2019

Closes #930

@alexandru
Copy link
Member

left a comment

Hey, thanks for the PR — we have 2 issues:

  1. we can't introduce operators that are inconsistent in their signature with Cats, see comment
  2. I'd rather have a single PR for both Task and Coeval
* // Result value will be "second"
* }}}
*/
final def *>[B](that: => Coeval[B]): Coeval[B] =

This comment has been minimized.

Copy link
@alexandru

This comment has been minimized.

Copy link
@valenterry

valenterry Jun 27, 2019

Author Contributor

@alexandru I can merge the PRs. Should we just rename *> to >> then or add both *> and >> with different semantics?

This comment has been minimized.

Copy link
@Avasil

Avasil Jun 27, 2019

Collaborator

@alexandru strict parameter is not safe with infinite recursion and it sometimes catches users off-guard in cats-effect / monix gitter channels. You can't explain it as just flatMap(_ => a) because of this catch

In my opinion if we go with one operator (to not pollute API too much) it's better to use *> exactly because the one from Cats has gotchas. And I think there is a law in cats that flatMap and map2 should be consistent so I'd prefer to have it without corner cases. :D

This comment has been minimized.

Copy link
@oleg-py

oleg-py Jun 27, 2019

Collaborator

I'd go with both sharks and >>, or only >>. IMO that's the least surprising operator that we should promote, and most of the times its "right-bias" is exactly what you need for quick effect combination that doesn't yet need for-comprehension.

This comment has been minimized.

Copy link
@Avasil

Avasil Jul 4, 2019

Collaborator

@oleg-py what makes <* better than << and what's your opinion on consistency vs safety? I'd rather avoid strict *> so either have it like in the PR or >> instead

This comment has been minimized.

Copy link
@oleg-py

oleg-py Jul 4, 2019

Collaborator

IMO having inconsistency that is safer only as long as you don't abstract Task/Coeval due to extension methods being shadowed isn't really safer. It'll eventually bite the user.

I didn't say that << is better, but it is both deprecated and strict in cats, so I'd not add it for same consistency reasons.

I'm fine with strict *> for cats consistency as long as we have non-strict >>.

This comment has been minimized.

Copy link
@Avasil

Avasil Jul 4, 2019

Collaborator

Okay, fair enough. I guess we can let @valenterry decide. :D
by-name >> only or >> with strict *> and <*

If we go with the latter, I would add a comment in scaladoc with a warning

This comment has been minimized.

Copy link
@valenterry

valenterry Jul 4, 2019

Author Contributor

I think I'd go for >> only then, even though it is a bad sad that << deprecated in cats.

This was referenced Jun 27, 2019
@valenterry

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@alexandru changed it accordingly, please re-review

@Avasil
Avasil approved these changes Jul 15, 2019

@Avasil Avasil merged commit c7aac9a into monix:master Jul 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.