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

Fix embarrassing traverse bugs 🙈 #34

Merged
merged 3 commits into from
Sep 26, 2021
Merged

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Sep 25, 2021

This fixes 3 bugs in traverse:

  1. For an empty product we have to still wrap it in pure.
    This wasn't caught because case objects are covered by Const.
  2. We were capturing the mutable variable i when the Applicative is lazy.
  3. We cannot use a mutable Array as an accumulator, e.g. when the Applicative is for List.

For an empty product we have to still wrap it in `pure`.
This wasn't caught because case objects are covered by `Const`.
@joroKr21 joroKr21 added the bug Something isn't working label Sep 25, 2021
@joroKr21 joroKr21 self-assigned this Sep 25, 2021
@joroKr21 joroKr21 changed the title Fix embarrassing traverse bug 🙈 Fix embarrassing traverse bugs 🙈 Sep 25, 2021
if (n == 0) x0

def prepend(xs: List[Any])(x: Any) = x :: xs
def fromList(xs: List[Any]) =
Copy link
Member Author

@joroKr21 joroKr21 Sep 25, 2021

Choose a reason for hiding this comment

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

Or use a Vector - I can't tell which is better 🤷 - it has to be immutable though

Copy link
Member

@milessabin milessabin left a comment

Choose a reason for hiding this comment

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

LGTM!

@joroKr21 joroKr21 merged commit 34fa0c4 into typelevel:main Sep 26, 2021
@joroKr21 joroKr21 deleted the traverse branch September 26, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants