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

Enable breakout in functions reduceRightToOption and reduceRightTo. #3498

Merged

Conversation

takayahilton
Copy link
Contributor

@takayahilton takayahilton commented Jun 25, 2020

now

import cats.data.NonEmptyList
import cats.implicits._

val notAllEven = NonEmptyList.of(2, 4, 6, 9, 10, 12, 14)
notAllEven.reduceMapA { a => println(a); if (a % 2 == 0) Some(a) else None }

14
12
10
9
6
4
2
res: Option[Int] = None

After applying this PR

import cats.data.NonEmptyList
import cats.implicits._

val notAllEven = NonEmptyList.of(2, 4, 6, 9, 10, 12, 14)
notAllEven.reduceMapA { a => println(a); if (a % 2 == 0) Some(a) else None }

2
4
6
9
res: Option[Int] = None

@takayahilton
Copy link
Contributor Author

ref: #3015

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2020

Codecov Report

Merging #3498 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3498   +/-   ##
=======================================
  Coverage   91.75%   91.75%           
=======================================
  Files         383      383           
  Lines        8406     8409    +3     
  Branches      232      217   -15     
=======================================
+ Hits         7713     7716    +3     
  Misses        693      693           

@takayahilton takayahilton force-pushed the fix-reduceRightTo-evaluation-order branch from 3811c48 to 90fde63 Compare June 26, 2020 16:39
@takayahilton takayahilton changed the title Fix the evaluation order of reduceRightTo/reduceRightToOption. Enable breakout in functions reduceRightToOption and reduceRightTo. Jun 26, 2020
Copy link
Contributor

@barambani barambani left a comment

Choose a reason for hiding this comment

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

What do you think about adding a couple of tests to make sure that this is enforced in future changes ? On the line of your examples

test("test") {
    val notAllEven = NonEmptyList.of(2, 4, 6, 9, 10, 12, 14)
    val out = mutable.ListBuffer[Int]()

    notAllEven.reduceMapA { a => out += a; if (a % 2 == 0) Some(a) else None }

    out.toList should ===(List(2, 4, 6, 9))
  }

May be also a stack safety check ? Thanks.

@@ -2,7 +2,6 @@ package cats
package data

import NonEmptyChainImpl.create
import cats.{Order, Semigroup}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cleanup here and below, thanks.

@takayahilton takayahilton force-pushed the fix-reduceRightTo-evaluation-order branch from 0a5996a to fba36f9 Compare July 5, 2020 06:18
Copy link
Contributor

@barambani barambani left a comment

Choose a reason for hiding this comment

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

thanks 👍

@djspiewak djspiewak merged commit 2838d3b into typelevel:master Jul 16, 2020
@takayahilton takayahilton deleted the fix-reduceRightTo-evaluation-order branch July 16, 2020 17:50
@travisbrown travisbrown added this to the 2.2.0-RC2 milestone Jul 21, 2020
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.

5 participants