Skip to content

Implements Enum.zip_with and Stream.zip_with #10419

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

Merged
merged 3 commits into from
Oct 13, 2020

Conversation

Adzz
Copy link
Contributor

@Adzz Adzz commented Oct 11, 2020

This is my first stab at this so comments very welcome.

Feature Request thread is here: https://groups.google.com/u/1/g/elixir-lang-core/c/mwBRUILsVHA

My basic approach was that the current zip/1 and zip/2 functions can be implemented by zip_with by doing this:

zip(enums) do
  zip_with(enums, &List.to_tuple/1)
end

To implement zip_with/2 the zip_fun you pass to it will get a list of each of the elements from each enumerable we are zipping. This seemed okay, but it differs from what zip/3 does, where each element from the collection being zipped is a different argument to the zipping fun.

I think that's okay but happy to change them to both be consistent with each other.


def zip_with(enumerables, zip_fun) do
Stream.zip_with(enumerables, zip_fun)
|> Enum.to_list()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is equivalent, but the Stream code lost me a bit...

@krainboltgreene
Copy link
Contributor

It would be useful to note that this is an endofunctor (the container type you put in is the container type you pull out).

end

def zip_with(enumerable1, enumerable2, fun) do
zip_with([enumerable1, enumerable2], fun)
Copy link
Member

Choose a reason for hiding this comment

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

The following example will fail:

iex> Enum.zip_with([1, 2, 5, 6], 3..4, fn x, y -> x + y end)

That's because we call zip_with and zip_with with pass a list to the fn. You can fix it by calling it like this:

Suggested change
zip_with([enumerable1, enumerable2], fun)
zip_with([enumerable1, enumerable2], &apply(fun, &1))

You can also take this approach on Stream.zip_with/3 so it calls fun with two args.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, please add tests mixing lists with ranges and similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice spot! Will do.

What's your thoughts on having zip_with/2's zipping fun receive a list instead?
Essentially turn this line: https://github.com/elixir-lang/elixir/pull/10419/files#diff-6881431a92cd4e3ea0de82bf2338f8eaR3797
into this:

  defp zip_list([h1 | next1], [h2 | next2], fun) do
   # pass a list here to fun
    [fun.([h1, h2]) | zip_list(next1, next2, fun)]
  end

I feel like it will be more consistent

@josevalim
Copy link
Member

This looks great, awesome job! Please note that the first paragraph of @doc must always be a one-line summary explaining what the function does. Can you please update them accordingly? Thanks.


@doc """
Zips corresponding elements from a finite collection of enumerables into a new enumerable which
is built by calling zip_fun with the first element from each enumerable, then the with the
Copy link
Contributor

Choose a reason for hiding this comment

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

"then the with the second element"
I think a word or more are missing

@Adzz
Copy link
Contributor Author

Adzz commented Oct 12, 2020

@krainboltgreene I don't think it is an endofunctor... take this example, you zip two maps but get returned a list. Like lots of the enum functions.

Enum.zip_with(%{a: 1}, %{a: 2}, fn {_, left}, {_, right} -> left + right end)
# [3]

@josevalim josevalim merged commit 1705984 into elixir-lang:master Oct 13, 2020
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@Adzz Adzz deleted the implement_zip_with/3 branch October 13, 2020 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants