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

Semigroup object additions #4625

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

igstan
Copy link
Contributor

@igstan igstan commented Jun 17, 2024

Adresses: #4615

@satorg
Copy link
Contributor

satorg commented Dec 5, 2024

It seems this PR was overlooked... Where do we stand on this?
Does it make sense to add some tests for the new methods added?

As I can see, there are some tests for the former static methods in SemigroupSuite. Perhaps, those tests don't look exactly comprehensive, but it's better than nothing.

@johnynek
Copy link
Contributor

johnynek commented Dec 5, 2024

I guess adding it costs little, although the style this represents (which we used in Algebird at twitter to simplify type inference) isn't used outside of algebra (which adopted it from algebird)

That said, since those files are using it, I can see the argument for being consistent and I think we are unlikely to add more methods to the traits needed many more updates.

I would be for it to improve consistency with existing code.

@@ -178,6 +178,11 @@ object Semigroup
*/
@inline def last[A]: Semigroup[A] = (_, y) => y

@inline def reverse[A](implicit ev: Semigroup[A]): Semigroup[A] = ev.reverse
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a huge benefit here since to use it you will often need Semigroup.reverse[String] compared to Semigroup[String].reverse which isn't a huge difference. You could avoid the type by passing to a method that expects a certain type, which may be common (since you are passing to an implicit/using in many cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. The reason I had added it was for consistency with the other methods.

In any case, I've removed the commit adding reverse and kept just the one adding intercalate.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could say the rule is: if the method takes a parameter of that is sufficient to infer the type of the result we add this method. In that way, it's still consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I think that's a pretty good rule. Thanks for your review.

@igstan igstan force-pushed the igstan/semigroup-object-additions branch from 99714f1 to 5db6c0f Compare December 24, 2024 04:44
@igstan igstan force-pushed the igstan/semigroup-object-additions branch from 5db6c0f to 291ef19 Compare December 24, 2024 04:47
@satorg satorg merged commit 9aed5c4 into typelevel:main Dec 24, 2024
16 checks passed
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.

4 participants