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

[PROPOSAL] Aliases for methods traverse_ and sequence_ #4611

Open
satorg opened this issue Jun 5, 2024 · 7 comments · May be fixed by #4682
Open

[PROPOSAL] Aliases for methods traverse_ and sequence_ #4611

satorg opened this issue Jun 5, 2024 · 7 comments · May be fixed by #4682
Labels
good first issue Issues that are easier to take on for first time contributors low-hanging fruit

Comments

@satorg
Copy link
Contributor

satorg commented Jun 5, 2024

Since fa.traverse_(fun) and fa.sequence_ are semantically the same as fa.traverse(fun).void and fa.sequence.void I would propose to add aliases traverseVoid and sequenceVoid for them correspondingly.

Why?

In two words: discoverability and clarity.

The names traverse_ and sequence_ look cryptic and not self-explaining at all.
I personally witnessed guys who were relative new to Cats and they were able to figure out regular traverse and sequence theirselves but were struggling with the traverse_ and sequence_ variants. Most likely this is because across the industry underscores in method names are often used to mark some kind of internal/system API which should not be normally in use.

@satorg satorg added the good first issue Issues that are easier to take on for first time contributors label Jun 6, 2024
@djspiewak
Copy link
Member

djspiewak commented Jun 7, 2024

Normally I hate having multiple words for things but… yeah, the underscore variants are really weird, particularly for something so commonly used. Agreed this is worth fixing. Also we've done this in a few places in Cats Effect as well, perhaps most notably async_ (though in our defense on that one, it is the variant you probably shouldn't be using).

@danicheg
Copy link
Member

danicheg commented Jun 7, 2024

As of my two cents, technically, that underscore postfix has been well-recognized and has influenced the whole ecosystem (not only the Typelevel one). We shouldn't give up on it, for sure.
But it's also true that adding a few more aliases will barely make things worse. To consider:

  • traverseUnit (for newcomers, it's explicit on the return type)
  • traverseDiscard (for newcomers, it's explicit on the intent)
  • traverseVoid (for those familiar with Functor#void, it's explicit on the intent)

@johnynek
Copy link
Contributor

johnynek commented Jun 7, 2024

Personally, I think traverseVoid is better than traverse_. It is a contraction of traverse(_).void and it hopefully helps the user also learn about .void.

While traverseUnit seems plausible, note that def unit: F[Unit] which is a constant, while def void[A](f: F[A]): F[Unit] is not.. so traverseUnit invites that confusion in my opinion.

Discard is also possibly nice, but if we go down that road, I think we want def discard[A](f: F[A]): F[Unit] added to functor as a synonym for void. Secondly, the "discard" part is just the inside value, whereas if you read discard(List(1, 2, 3)) it's not obvious that is just discarding the inside, not the entire thing (not sure why you would want that, but still, just pointing out that to someone who doesn't know the functions it may still be unclear).

In summary, my vote would be that making traverseVoid as an alias for traverse_ would be a positive move without introducing a larger question of re-litigating def void.

@rossabaker
Copy link
Member

I like traverseVoid, too.

Here's my question: do we deprecate the old and create toil, or live with the duplicate names until the heat death of the universe and/or Cats 3?

@satorg
Copy link
Contributor Author

satorg commented Jun 8, 2024

I think we can arrange something in the middle. For example:

  1. Add the aliases but do not deprecate the old method names.
  2. When cutting the next release, mention in the release notes, that we're going to deprecate those methods soon.
  3. After the next release create a follow-up PR that deprecates the old names, but do not merge it. Keep it unmerged until two or three releases get passed.
  4. Once we feel that the majority of downstream projects are ready, merge the PR eventually.
  5. Cut yet another release.

A plan like that might work, I guess.

@joroKr21
Copy link
Member

joroKr21 commented Jun 8, 2024

I think it's possible to offer a scalafix rewrite that could even be applied by scala-steward. This could be done together with deprecating the old methods.

@satorg
Copy link
Contributor Author

satorg commented Jun 9, 2024

Agree, a scalafix rule could come in handy indeed. I would suggest though to address it in a separate follow-up PR, i.e.:

  1. Introduce aliases;
  2. Create the scalafix rule and deprecate the old names.

Just to let the aliases go live without awaiting the rewrite to come out. I feel we can expedite the whole process that way.

That said, I also feel that in this particular case for many users it could be way easier to do a simple "find-and-replace" across their projects rather than bother with scalafix. Especially if there's no automated process of applying scalafix rules in there (which I bet many real-world projects don't have).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are easier to take on for first time contributors low-hanging fruit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants