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

Any reason intercalate isn't exposed directly on the Semigroup companion object? #4615

Open
igstan opened this issue Jun 10, 2024 · 3 comments

Comments

@igstan
Copy link
Contributor

igstan commented Jun 10, 2024

Is there a reason not to expose the intercalate method directly on the Semigroup object?

object Semigroup {
  @inline def intercalate[A](sep: A)(implicit ev: Semigroup[A]): Semigroup[A] =
    ev.intercalate(sep)
}

It provides some type inference convenience in that we can now write Semigroup.intercalate(";"), for example, instead of Semigroup[String].intercalate(";").

Am I missing something or is it just an oversight?

@satorg
Copy link
Contributor

satorg commented Jun 10, 2024

I don't think it is an oversight, but rather a very particular case that just has never been in demand previously.
For example, there's another method reverse that also construct a Semigroup instance out of the current one, but adding its counterpart to the object wont' help with type inference a lot:

Semigroup[String].reverse
// VERSUS
Semigroup.reverse[String]

When it comes to intercalate, then I agree it can come in handy, so I think it wouldn't hurt if we added it.

@johnynek
Copy link
Contributor

It was an oversight. This style of typeclass encoding was used in the algebra project which predates cats and was absorbed into cats.

The motivation is that if you have def combine[A](a: A, b: A)(implicit ev: Semigroup[A]): A = ev.combine(a, b) you can avoid declaring the type, and get good JVM performance (the jit should be able to remove the indirection since is is statically the same as: implicitly[Semigroup[A]].combine(a, b) but without naming the type.

Semigroup.combine("foo", "bar")

Cats used an approach of just giving def apply[A: Semigroup]: Semigroup[A] ... but that requires naming the type as noted above.

There is a lot of boilerplate with the approach of duplicating methods from the typeclass into the object in order to hopefully get some nice type-inference cases. Personally, I have moved away from the style in most cases.

That said, I'm not opposed to adding it.

@igstan
Copy link
Contributor Author

igstan commented Jun 10, 2024

Thanks, both. As it stands right now, it seems that only reverse and intercalate aren't exposed on the object, while the rest are (i.e., combine, combineN, combineAllOption). In addition, first, last and a few others are only available via the object.

What I'd do is add both intercalate and reverse on the object. The advantage is obvious for intercalate, while for reverse it would be just for consistency and because it doesn't take anything away DX-wise.

Does that sound good? If yes, I'll open a PR with both reverse and intercalate.

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

No branches or pull requests

3 participants