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

Remove Monoid and Semigroup combine from replacements #3003

Merged
merged 10 commits into from
Mar 30, 2023

Conversation

gutiory
Copy link
Collaborator

@gutiory gutiory commented Mar 28, 2023

This PR replace any method that belongs to deprecated Monoid or Semigroup that are in the deprecation replacements.

The last commit just deprecates the foldMap method in Fold interface. I'd need extra 👀 on this, because a bunch of other interfaces implement this interface. Basically, I've done the following:

  • Deprecated the foldMap implementation that has a Monoid parameter
  • Remove the foldMap implementation that does not have the Monoid, but that creates one and used the deprecated foldMap. This makes this method abstract.
  • Every interface that uses Fold implements now the abstract foldMap.

@gutiory gutiory requested review from serras, nomisRev and franciscodr and removed request for serras and nomisRev March 28, 2023 15:23
@serras
Copy link
Member

serras commented Mar 28, 2023

This PR replace any method that belongs to deprecated Monoid or Semigroup that are in the deprecation replacements.

The last commit just deprecates the foldMap method in Fold interface. I'd need extra 👀 on this, because a bunch of other interfaces implement this interface. Basically, I've done the following:

* Deprecated the `foldMap` implementation that has a `Monoid` parameter

* Remove the `foldMap` implementation that does not have the `Monoid`, but that creates one and used the deprecated `foldMap`. This makes this method abstract.

* Every interface that uses `Fold` implements now the abstract `foldMap`.

Sorry if this was not clear from the original PR, but we decided against taking that route. The reason is that we break binary compatibility. The breaking change of actually making foldMap with combine and empty the "main" version is for 2.0.

@@ -904,7 +904,7 @@ public sealed class Either<out A, out B> {

@Deprecated(
NicheAPI + "Prefer when or fold instead",
ReplaceWith(" fold({ MN.empty() }, f)")
ReplaceWith(" fold({ empty }, f)")
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused by these replacements. This won't result in correct code, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it won't work. I'm writing an entry in the arrow web migration guide just to explain this. It's the only way to get rid of the Monoid.empty deprecated method.

Copy link
Member

@serras serras left a comment

Choose a reason for hiding this comment

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

Looking forward to seeing how the migration guide looks like :)

Copy link
Collaborator

@franciscodr franciscodr left a comment

Choose a reason for hiding this comment

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

Regarding the Monoid empty value, I see we name it differently in some ReplaceWith expressions, like empty, initial, or initialValue. We could choose a unique identifier and use it in all the examples for consistency.

@gutiory
Copy link
Collaborator Author

gutiory commented Mar 30, 2023

Regarding the Monoid empty value, I see we name it differently in some ReplaceWith expressions, like empty, initial, or initialValue. We could choose a unique identifier and use it in all the examples for consistency.

Absolutely, @franciscodr . I've mixed some names from Monoid.empty() and others from the fold parameter.
What about to rename all with the name of the fold parameter we're suggesting to be replaced with?

@franciscodr
Copy link
Collaborator

Regarding the Monoid empty value, I see we name it differently in some ReplaceWith expressions, like empty, initial, or initialValue. We could choose a unique identifier and use it in all the examples for consistency.

Absolutely, @franciscodr . I've mixed some names from Monoid.empty() and others from the fold parameter. What about to rename all with the name of the fold parameter we're suggesting to be replaced with?

Yeah, makes sense to me

Copy link
Collaborator

@franciscodr franciscodr left a comment

Choose a reason for hiding this comment

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

Thanks, @gutiory!

@gutiory gutiory merged commit ec0d786 into main Mar 30, 2023
@franciscodr franciscodr deleted the jg-remove-combine-in-deprecations branch March 30, 2023 11:15
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.

3 participants