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

Rename traverse_ and sequence_ in Foldable #1723

Closed
LukaJCB opened this issue Jun 8, 2017 · 11 comments
Closed

Rename traverse_ and sequence_ in Foldable #1723

LukaJCB opened this issue Jun 8, 2017 · 11 comments

Comments

@LukaJCB
Copy link
Member

LukaJCB commented Jun 8, 2017

I realize the underscore comes from the Haskell equivalent, but I do not believe it conveys its intent.
The two functions do the same as their non-underscore counterparts, but ignore the result returning F[Unit].

I suggest something along the lines of traverseVoid or traverseToUnit.

This would also affect the Reducible type class and its non empty counterparts.

Edit: Something to keep in mind, is that these two methods are already being renamed in Reducible as part of #1611 (traverse1_ -> nonEmptyTraverse_). So we're going to be breaking the API anyway. I think it would make a lot of sense to use this opportunity to rename these and give them a better name.

@diesalbla
Copy link
Contributor

diesalbla commented Jun 10, 2017

Alternative Proposal

The type-class Functor provides a function void, which transforms an F[A] to an F[Unit]; and its syntax class exports it. Thus, you can write xs.traverse_(foo) as xs.traverse(foo).void, which IMHO better conveys the meaning with just four more characters.

I suggest directly removing the methods traverse_ and sequence_.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 10, 2017

I have to disagree there, traverse has a Traverse typeclass constraint, while traverse_ only requires Foldable.

@diesalbla
Copy link
Contributor

You are right. I just thought, by the similarity of the names, that the traverse_ method would be defined in the Traverse type-class, but I was wrong and that is not the case.

I agree they should be renamed, but I would avoid using the word traverse in the new names. Since the goal is to iterate through each element in the collection in order, perform an action (a Unit value with an effect), I would then suggest calling them foreach or foreachDo, or foldEach; that would be closer to the same-named methods in the List class of the standard library.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 12, 2017

I think it makes sense to keep the traverse and sequence naming scheme as it's well established by now. I just disagree with the underscore. We could also think about calling it traverseIgnoreResult or something along these lines.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 21, 2017

I think that something like traverseVoid is probably a bit better than traverse_. However, I personally don't feel strongly enough about this to make an API change. I'm open to others' opinions on this. @johnynek you are someone who has requested that we don't change APIs in the past. Do you have any thoughts on this?

@mdedetrich
Copy link

mdedetrich commented Jul 28, 2017

I actually like the name of traverseUnit as it sounds more idiomatic with Scala. However @diesalbla has a point, in which case if you wanted to follow examples for stdlib you would call it foreach (if this is an argument in regards to following the stdlib)

@diesalbla
Copy link
Contributor

diesalbla commented Jul 28, 2017

The names for traverse and sequence are well established in the reference literature, from the Haskell libraries. However, I would not use at all the word traverse in the name of this method, because it suggests a link to the Traverse typeclass, which @LukaJCB pointed out does not exist.

It is worth noting that, as discussed here, the Haskell libraries did not include a Foldable (or Reducible) typeclass until a recent version, long after the first release. Presumably, that is why these methods were in the Traverse type-class at first, which in turn explains why they were called like that. That being the case, since cats is not (yet) released, IMHO we should take the chance not to use the words traverse or sequence for this methods.

Apart from foreach for traverse_, I would also suggest using doAll for sequence_. On the other hand, since traverse_ and sequence_ are well-known, we should keep them as deprecated aliases FTTB .

@mdedetrich
Copy link

Also we shouldn't just be copying Haskell in this regard, because both libraries have different naming schemes. The most obvious is how map/flatMap is called in Scala is completely different to how its called in Haskell.

I think the attempt should be for cats to actually try and use Scala's idiomatic naming scheme, in which case foreach is probably what we want.

@SystemFw
Copy link
Contributor

SystemFw commented Jul 29, 2017

Using the same name as the standard library means that syntax stops working.
Now you can do List(1,2,3).traverse_(a => IO(println(a)), if you change traverse_ to foreach, the existing stdlib method will be selected, forcing you to do Foldable[List].foreach(List(1,2,3))(a => IO(println(a)).
For this reason, I think cats should actually strive to not call things like in the stdlib.

@diesalbla
Copy link
Contributor

@LukaJCB Are you still interested in pushing for this?

@rossabaker
Copy link
Member

Didn't achieve critical mass, and the names are further entrenched now. Let's close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants