Implement Access.filter/1 #6634

zachdaniel · 2017-10-04T21:34:36Z

Implements Access.find/1 and Access.filter/1. Examples and documentation provided in the code (aligned with examples and documentation for Access.at/1.

crestenstclair · 2017-10-04T21:56:03Z

This looks great, and extremely useful.

zachdaniel · 2017-10-04T21:56:52Z

I checked on PR #6364 and saw that it restructures documentation, so I haven't touched the main documentation, only the documentation specific to the functions I'm adding.

josevalim · 2017-10-04T22:11:35Z

lib/elixir/lib/access.ex

@@ -700,4 +700,158 @@ defmodule Access do
  defp get_and_update_at([], _index, _next, updates) do
    {nil, :lists.reverse(updates)}
  end
+
+  @doc ~S"""
+    Returns a function that accesses all elements of a list that match the provided predicate.


The whole docs should be two spaces.

josevalim · 2017-10-04T22:12:17Z

lib/elixir/lib/access.ex

+
+    ## Examples
+
+        iex> list = [%{name: "john", salary: 10}, %{name: "mary", salary: 20}, %{name: "francine", salary: 30}]


We can probably do it with a list of two to simplify examples.

I ended up going with a list of 3 items to show order preservation, and specifically with filter to show how order is preserved on both sides. I don't mind changing it though, if you feel strongly about it.

I could also write a non-example test to confirm that, and use the simpler example?

I could also write a non-example test to confirm that, and use the simpler example?

Perfect.

josevalim · 2017-10-04T22:12:52Z

lib/elixir/lib/access.ex

+        iex> get_in(%{}, [Access.filter(fn a -> a == 10 end)])
+        ** (RuntimeError) Access.filter/1 expected a list, got: %{}
+  """
+  def filter(func) when is_function(func, 1) do


It is better to not check the arity because that leads to a better error later on the invocation site.

Good point, I'll change now.

josevalim · 2017-10-04T22:16:44Z

Thank you @zachdaniel! I like filter a lot but i am not convinced on find since find can be seen as a equivalent to filter on all cases where the finding criteria is unique. So I would like to merge filter and we can wait until folks request something like find. Wdyt?

zachdaniel · 2017-10-04T22:19:13Z

@josevalim I agree. I really use Access.filter/1 often, and I added Access.find/1 just in addition while I was at it. It could also be easily simulated w/ get_in(list, [Access.filter(pred), Access.at(0)])

zachdaniel · 2017-10-04T22:19:50Z

Although on large lists, it would perform much worse. Either way, I'm on board to remove find

josevalim · 2017-10-04T22:24:39Z

lib/elixir/lib/access.ex

+    raise "Access.filter/1 expected a list, got: #{inspect data}"
+  end
+
+  defp get_and_update_where([head | rest], func, next, updates, gets) do


let's rename where to filter here and below then please? thanks!

zachdaniel · 2017-10-04T22:35:43Z

Sounds good. I'll make the following changes when I get home tonight:

Rename get_and_update_where to get_and_update_filter
Simplify examples and add more complex supporting tests.

zachdaniel · 2017-10-05T03:23:00Z

@josevalim The above items are complete.

josevalim · 2017-10-05T12:13:54Z

lib/elixir/test/elixir/access_test.exs

+    @test_list [1, 2, 3, 4, 5, 6]
+
+    test "filters in get_in" do
+      assert(get_in(@test_list, [Access.filter(&(&1 > 3))]) == [4, 5, 6])


Can you please remove the parens around assert on this test and the ones below?

ericmj · 2017-10-05T12:14:15Z

lib/elixir/test/elixir/access_test.exs

+
+    test "chains with other access functions" do
+      mixed_map_and_list = %{foo: Enum.map(@test_list, &(%{value: &1}))}
+      assert(get_in(mixed_map_and_list, [:foo, Access.filter(&(&1 <= 3)), :value]) == [])


This test is a bit confusing. Maybe it would be better to filter based on is_tuple/1?

Not sure exactly what you mean. None of the elements of the list are tuples. get_and_update_in returns a tuples of the get value and update value though. I think using the @test_list module attribute is just less clear,and maybe it would be clearer with an explicit test list in each test.

Your filter is comparing a map with <= 3. If you want to reject everything, it would be better to have a filter based on the data structure.

Sorry, is_tuple was wrong, is_map is probably better. You do &(&1 <= 3) where &1 is a map which doesn't really make sense. Is %{value: 1} <= 3 the comparison you wanted to make? It would be better if you changed it to a more natural comparison.

Yeah, sorry it was supposed to be &1.value <= 3 I'm pretty sure the test should be failing with that mistake though.

It doesn't fail because all terms are comparable, even across different types.

Oh, it's just wrong all around. Shouldn't be asserting an empty list. I'll fix now.

When I said it should fail I was thinking that I had the right side of the comparison right [1, 2, 3]

zachdaniel · 2017-10-05T19:00:30Z

@josevalim @ericmj Those two issues are addressed. I didn't simplify the test really, but just fixed it to use &1.value

josevalim · 2017-10-05T19:29:14Z

❤️ 💚 💙 💛 💜

albertored · 2018-08-01T11:09:53Z

@josevalim @zachdaniel (sorry for explicit mention but I don't know if you receive notification for closed PR)

I really would like to have also Access.find/1. Not only for performance reason but also because it can not be replaced with [Access.filter(pred), Access.at(0)], see for example

iex> data = %{a: [%{id: 1, age: 4}, %{id: 2, age: 5}]}

iex> get_in(data, [:a, Access.find(fn el -> el.id == 1 end)])
%{age: 4, id: 1}

iex> get_in(data, [:a, Access.filter(fn el -> el.id == 1 end)])
[%{age: 4, id: 1}]

 iex> get_in(data, [:a, Access.filter(fn el -> el.id == 1 end), Access.at(0)])
** (RuntimeError) Access.at/1 expected a list, got: %{age: 4, id: 1}

added find and where

90b0fb9

zachdaniel changed the title ~~added find and where~~ Added Access.find/1 and Access.where/1 Oct 4, 2017

zachdaniel changed the title ~~Added Access.find/1 and Access.where/1~~ Implement Access.find/1 and Access.where/1 Oct 4, 2017

zachdaniel changed the title ~~Implement Access.find/1 and Access.where/1~~ Implement Access.find/1 and Access.filter/1 Oct 4, 2017

Update access.ex

452e0ba

josevalim reviewed Oct 4, 2017

View reviewed changes

zachdaniel added 3 commits October 4, 2017 17:23

fixing documentation indentation

6994330

fixed function arity check

1f05698

responding to feedback

ca929a4

josevalim reviewed Oct 4, 2017

View reviewed changes

renaming private function, add supporting tests

db14fc8

zachdaniel changed the title ~~Implement Access.find/1 and Access.filter/1~~ Implement Access.filter/1 Oct 5, 2017

Update access_test.exs

195a85f

josevalim approved these changes Oct 5, 2017

View reviewed changes

josevalim reviewed Oct 5, 2017

View reviewed changes

ericmj reviewed Oct 5, 2017

View reviewed changes

fixing confusing test, and formatting

771bbf9

ericmj approved these changes Oct 5, 2017

View reviewed changes

josevalim merged commit 3dca96f into elixir-lang:master Oct 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Access.filter/1 #6634

Implement Access.filter/1 #6634

zachdaniel commented Oct 4, 2017 •

edited

crestenstclair commented Oct 4, 2017

zachdaniel commented Oct 4, 2017

josevalim Oct 4, 2017

josevalim Oct 4, 2017

zachdaniel Oct 4, 2017

zachdaniel Oct 4, 2017

josevalim Oct 4, 2017

josevalim Oct 4, 2017

zachdaniel Oct 4, 2017

josevalim commented Oct 4, 2017

zachdaniel commented Oct 4, 2017

zachdaniel commented Oct 4, 2017

josevalim Oct 4, 2017

zachdaniel commented Oct 4, 2017

zachdaniel commented Oct 5, 2017

josevalim Oct 5, 2017

ericmj Oct 5, 2017

zachdaniel Oct 5, 2017

josevalim Oct 5, 2017

ericmj Oct 5, 2017 •

edited

zachdaniel Oct 5, 2017 •

edited

ericmj Oct 5, 2017 •

edited

zachdaniel Oct 5, 2017

zachdaniel Oct 5, 2017

zachdaniel commented Oct 5, 2017

josevalim commented Oct 5, 2017

albertored commented Aug 1, 2018 •

edited


		## Examples

		iex> list = [%{name: "john", salary: 10}, %{name: "mary", salary: 20}, %{name: "francine", salary: 30}]

Implement Access.filter/1 #6634

Implement Access.filter/1 #6634

Conversation

zachdaniel commented Oct 4, 2017 • edited

crestenstclair commented Oct 4, 2017

zachdaniel commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 4, 2017

zachdaniel commented Oct 4, 2017

zachdaniel commented Oct 4, 2017

Choose a reason for hiding this comment

zachdaniel commented Oct 4, 2017

zachdaniel commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmj Oct 5, 2017 • edited

Choose a reason for hiding this comment

zachdaniel Oct 5, 2017 • edited

Choose a reason for hiding this comment

ericmj Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachdaniel commented Oct 5, 2017

josevalim commented Oct 5, 2017

albertored commented Aug 1, 2018 • edited

zachdaniel commented Oct 4, 2017 •

edited

ericmj Oct 5, 2017 •

edited

zachdaniel Oct 5, 2017 •

edited

ericmj Oct 5, 2017 •

edited

albertored commented Aug 1, 2018 •

edited