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

Invalid behaviour of Pathex.delete! when using concatenation and/or all lense #20

Closed
ffloyd opened this issue Oct 19, 2022 · 5 comments
Closed

Comments

@ffloyd
Copy link

ffloyd commented Oct 19, 2022

Let's say I have the following data:

data = %{
  "key" => "value",
  "list" => [
    %{
      "a" => 1,
      "b" => 2
    },
    %{
      "a" => 3,
      "b" => 4
    }
  ]
}

and my goal is to delete all "a" keys from all maps in the inner list. So, I create a path for them:

all_a = path("list") ~> Pathex.Lenses.all() ~> path("a")

Pathex.get(data, all_a)
# result:
[1, 3]

But when I use delete! it just wipes all items in the list:

Pathex.delete!(data, all_a)
# result:
%{"key" => "value", "list" => []}

Then I tried to not use the lens, but still do concatenation:

first_a_concat = path("list") ~> path(0) ~> path("a")

Pathex.get(data, first_a_concat)
# result:
1

Pathex.delete!(data, first_a_concat)
# result:
%{"key" => "value", "list" => [%{"a" => 3, "b" => 4}]} # it removes first item from the list!

When I use path without concatenation it works as it should:

first_a_path = path("list" / 0 / "a")

Pathex.get(data, first_a_path)
# result:
1

Pathex.delete!(data, first_a_path)
# result:
%{"key" => "value", "list" => [%{"b" => 2}, %{"a" => 3, "b" => 4}]}

Then I tried to check what will happen with some deeeep data:

deep_data = %{
  "list" => [
    %{
      "list" => [
        %{
          "list" => [
            %{
              "a" => 1,
              "b" => 2
            }
          ]
        }
      ]
    }
  ]
}

deep_dive_path =
  path("list")
  ~> Pathex.Lenses.all()
  ~> path("list")
  ~> Pathex.Lenses.all()
  ~> path("list")
  ~> Pathex.Lenses.all()
  ~> path("a")

Pathex.get(deep_data, deep_dive_path)
# result:
[[[1]]] # btw, shouldn't it be a flat list here?

Pathex.delete!(deep_data, deep_dive_path)
# result:
** (CaseClauseError) no case clause matching: :delete_me
    (pathex 2.4.0) lib/pathex/lenses/all.ex:111: anonymous fn/3 in Pathex.Lenses.All.all/0
    (elixir 1.14.0) lib/enum.ex:4751: Enumerable.List.reduce/3
    (elixir 1.14.0) lib/enum.ex:2514: Enum.reduce_while/3
    (pathex 2.4.0) lib/pathex/lenses/all.ex:110: anonymous fn/2 in Pathex.Lenses.All.all/0
    (stdlib 3.17.2) erl_eval.erl:672: :erl_eval.do_apply/5
    (stdlib 3.17.2) erl_eval.erl:270: :erl_eval.expr/5
    (pathex 2.4.0) lib/pathex/lenses/all.ex:111: anonymous fn/3 in Pathex.Lenses.All.all/0
    (elixir 1.14.0) lib/enum.ex:4751: Enumerable.List.reduce/3

So, it looks like delete!/2 has some real problems with lenses and concatenation. =)

@hissssst
Copy link
Owner

Yeah, looks like a bug in concatenation of delete operation

@hissssst
Copy link
Owner

hissssst commented Oct 20, 2022

Yeah, there was a tiny typo in concat.ex

But you're kinda misusing the library. all() lens works in an eager-style evaluation. Therefore. this lens either deletes all the values, or returns an error. For example,

delete(list, all() ~> path("a"))

returns{:ok, []} if all elements of the list have "a" as a key or :error.

To work with a more lazy-style evalutation I'd suggest using star() lens. And I think, your deep_dive_path should be implemented using star lens.

@hissssst
Copy link
Owner

@ffloyd , you can use version 2.4.1

@ffloyd
Copy link
Author

ffloyd commented Oct 20, 2022

Thank you! I am impressed with how fast you reacted! Appreciate it!

I checked the behavior for the updated version. With the star/0 lens everything works as expected. But I still see some weird behavior with all/0 lens:

import Pathex

data = [
  %{ "a" => 1 },
  %{ "a" => 2 }
]

all_a = Pathex.Lenses.all() ~> path("a")

Pathex.get(data, all_a) |> IO.inspect(label: "get")
Pathex.delete!(data, all_a) |> IO.inspect(label: "delete!")

And as result get/2 works, but delete/2 fails:

** (Pathex.Error) 
  Couldn't find element

    Path:      all() ~> path("a")

    Structure: [%{"a" => 1}, %{"a" => 2}]

    (stdlib 3.17.2) erl_eval.erl:685: :erl_eval.do_apply/6
    (stdlib 3.17.2) erl_eval.erl:893: :erl_eval.expr_list/6
    (stdlib 3.17.2) erl_eval.erl:408: :erl_eval.expr/5
    (elixir 1.14.0) lib/module/parallel_checker.ex:100: Module.ParallelChecker.verify/1

@hissssst
Copy link
Owner

@ffloyd , opened #21 to track the issue

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

No branches or pull requests

2 participants