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

Add an If-Then-Else operation to the Apply. #2609

Merged
merged 8 commits into from
Jan 12, 2019
Merged

Add an If-Then-Else operation to the Apply. #2609

merged 8 commits into from
Jan 12, 2019

Conversation

diesalbla
Copy link
Contributor

@diesalbla diesalbla commented Nov 13, 2018

The FlatMap type-class includes a function ifM, which allows evaluating a Boolean computation and, depending on the result, choosing between one effectful computation or other.

We add to the Apply type-class a similar function, ifA, which 1) takes as inputs an F[Boolean] and two F[A], and 2) depending on the value of the boolean result, returns either the first or the second F[A] on the result.

Although the profile is similar to ifM, they need to be separate functions due to the different way they handle the effects:

  • The ifM performs the effects of the condition F[Boolean], and the effects of one of the branches. Which branch, depends on the boolean result.
  • The ifA will always perform the effect of the F[Boolean], those of the first F[A], and those of the second F[A], in that order. It will apply the if-then-else selection to the values.

In other words, ifA only maps the values, but does not allow skipping effects.

The `FlatMap` type-class includes a function `ifM`, which allows
evaluating a Boolean computation and, depending on the result,
choosing between one effectful computation or other.

We add to the `Apply` type-class a very similar function.

Although the function is similar to `ifM`, there is an important
difference between them, as to how they handle the effect:
- The `ifM` will run the effects of the condition `F[Boolean]`,
  followed by the effects of _only one_ of the branches.
- The `ifA` will always run the effect of the condition, that of the first
  branch, and that of the second branch, in that order.

Thus, the if-then-else part in `ifA` only applies to the inner values,
not to the effects.
* scala> val asInt3: Option[Int] = b3.ifA(Some(1), None)
* asInt2: Option[Int] = None
*
* }}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we can't add any methods to type classses without breaking binary compatibility. So for now, we have to settle adding to a new syntax trait. Sorry I am on my phone and couldn't point you to an example.

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 If I understand you correctly, we can not add the method to the trait Apply[F[_]] (the type-class) but can only leave it as part of a syntactic extension implicit class?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the delayed response. Yes, that's the idea. You already updated the PR, we also need to create a new ApplySyntaxBinCompat trait to include an implicit conversion from F[Boolean] to IfAOps, then there are a couple of more steps,

  1. here we need to add a new trait AllSyntaxBinCompat4 to extends the new ApplySyntaxBinCompat,
    let the AllSyntaxBinCompat at the top extend this AllSyntaxBinCompat4
  2. [here] https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/implicits.scala we also need to let the implicits object extends this new AllSyntaxBinCompat4
  3. then we can probably add a doctest here that import cats.implicits._ to validate that it's all hooked up.

Sorry about the gymnastics you have to go through to overcome the Scala 2.11 binary compatibility restrictions.

diesalbla and others added 3 commits November 14, 2018 09:47
To preserve binary compatibility, we can not add methods to
an existing type-class, and can only use the syntax extension.
@@ -77,3 +78,6 @@ trait AllSyntaxBinCompat2
with ValidatedSyntaxBincompat0

trait AllSyntaxBinCompat3 extends UnorderedFoldableSyntax with Function1Syntax

trait AllSyntaxBinCompat4 extends IfApplyOps
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we need to extends ApplySyntaxBincompat0 instead

@kailuowang
Copy link
Contributor

after you are done with the changes, please also run sbt prePR which reformats the code.

@codecov-io
Copy link

codecov-io commented Nov 30, 2018

Codecov Report

Merging #2609 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2609      +/-   ##
==========================================
+ Coverage   95.15%   95.16%   +<.01%     
==========================================
  Files         365      365              
  Lines        6774     6777       +3     
  Branches      297      297              
==========================================
+ Hits         6446     6449       +3     
  Misses        328      328
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/apply.scala 57.14% <100%> (+32.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4d1ad1...2517183. Read the comment docs.

kailuowang
kailuowang previously approved these changes Nov 30, 2018
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

LukaJCB
LukaJCB previously approved these changes Nov 30, 2018
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks @diesalbla, looks great! :)

@kailuowang kailuowang dismissed stale reviews from LukaJCB and themself via 1d627c5 January 8, 2019 15:48
kailuowang
kailuowang previously approved these changes Jan 8, 2019
@kailuowang kailuowang merged commit 974cec1 into typelevel:master Jan 12, 2019
@LukaJCB
Copy link
Member

LukaJCB commented Jan 12, 2019

Thanks @diesalbla!

@diesalbla diesalbla deleted the applicative_if branch January 19, 2019 18:40
@diesalbla diesalbla mentioned this pull request May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants