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

deprecate combineAll in favour of fold #2687

Merged
merged 13 commits into from Mar 15, 2022
Merged

deprecate combineAll in favour of fold #2687

merged 13 commits into from Mar 15, 2022

Conversation

i-walker
Copy link
Member

resolves #2682

@i-walker i-walker requested a review from nomisRev March 10, 2022 14:33
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this @i-walker. We should do the same for Sequence.

i-walker and others added 2 commits March 14, 2022 09:57
@i-walker
Copy link
Member Author

Agree, I‘ll take of it in this PR

@i-walker i-walker changed the title deprecate combineAll deprecate combineAll in favour of fold Mar 15, 2022
…cate-combineAll

# Conflicts:
#	arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Iterable.kt
@i-walker i-walker requested a review from nomisRev March 15, 2022 09:47
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

I think we can reduce the API a bit, by promoting better usage of Monoid.
What do you think @i-walker. cc\ @raulraja

@@ -1496,7 +1496,11 @@ public fun <A, B> Either<A, B>.combine(SGA: Semigroup<A>, SGB: Semigroup<B>, b:
}
}

@Deprecated("use fold instead", ReplaceWith("fold(MA, MB)", "arrow.core.fold"))
Copy link
Member

Choose a reason for hiding this comment

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

Should instead of adding this new fold method deprecate towards fold(Monoid.either(MA, MB))?
These overloads seem a bit redundant.

Copy link
Member

Choose a reason for hiding this comment

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

I also think these overloads are redundant if we already have one instance for Either that we could take as a single argument.

I never use Monoid if I can avoid it and prefer a method that takes an empty value and an assoc function. That method already exists for Iterable, and it's just fold. In actual use cases in Kotlin, people are not going to provide their own Monoid or Semigroup instances to pass them around. These instances we provide are mainly for primitive types, but most use cases have more complex models inside the Iterable. Since users would not bother to construct complicated instances for Monoid over their model, these functions quickly become useless and need to be rewritten to basic fold or similar.

@i-walker i-walker requested a review from nomisRev March 15, 2022 11:59
Copy link
Member

@nomisRev nomisRev 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 @i-walker 🙌

@i-walker i-walker merged commit 6fe87fe into main Mar 15, 2022
@i-walker i-walker deleted the deprecate-combineAll branch March 15, 2022 13:45
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.

["Request"] Iterable::fold and Iterable::combineAll are duplcates
3 participants