Implement Enum.with_index/2 #4040
Conversation
|
Thank you @GeorgeTaveras1231! I have added a comment, we also need tests and we are good to go. |
| map_reduce(enumerable, 0, fn x, acc -> | ||
| @spec with_index(t) :: [{element, integer}] | ||
| def with_index(enumerable), do: with_index(enumerable, 0) | ||
| def with_index(enumerable, offset) do |
There was a problem hiding this comment.
It'd better to use defaults: offset \\ 0.
There was a problem hiding this comment.
@lexmag I was considering doing this but not sure what the benefit is, would you mind explaining your POV?
There was a problem hiding this comment.
@GeorgeTaveras1231 technically it is completely the same what you've done, but obviously less code and that's the common practice throughout codebase (I believe it is true for every Elixir project), thus there is no reason not to use it. 
There was a problem hiding this comment.
The benefit is what it would be considered one function, which is useful in the documentation. For example, in your current patch, you are only documenting with_index/1, with_index/2 would show with no documentation.
There was a problem hiding this comment.
Otherwise with current patch extra body-less function needs to be defined. Even this won't combine them for docs.
There was a problem hiding this comment.
Makes sense, thank you guys :)
|
We also need to change |
|
Thank you for the feedback @josevalim @lexmag! I will address the concerns some time today. |
GeorgeTaveras1231
force-pushed
the
feature/Enum.with_index/2
branch
from
b5e8e2d
to
7250002
Compare
| @spec with_index(t) :: [{element, non_neg_integer}] | ||
| def with_index(enumerable) do | ||
| map_reduce(enumerable, 0, fn x, acc -> | ||
| @spec with_index(t) :: [{element, integer}] |
There was a problem hiding this comment.
whoops, forgot to update the signature in the spec
This removes the previous implementation `Enum.with_index/1` but maintains backwards compatibility by providing a default offset of 0 Proposal at elixir-lang#4039
GeorgeTaveras1231
force-pushed
the
feature/Enum.with_index/2
branch
from
7250002
to
2d58208
Compare
This removes the previous implementation `Stream.with_index/1` but maintains backwards compatibility by providing a default offset of 0
|
@josevalim @lexmag what do you think? |
…ex/2 Implement Enum.with_index/2
|
❤️ 💚 💙 💛 💜 |
Proposal at #4039
TODO:
Stream.with_index/2