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

Add get_in/1 with safe nil-handling for access and structs #13370

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

josevalim
Copy link
Member

No description provided.

assert get_in(is_nil.age) == nil

assert_raise KeyError, ~r"key :unknown not found", fn -> get_in(users.unknown) end
end
Copy link
Member

Choose a reason for hiding this comment

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

it's probably redundant but I'd consider adding:

Suggested change
end
assert_raise KeyError, ~r"key :unknown not found", fn -> get_in(users.meg.unknown) end
end

lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved
lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved
@srcrip
Copy link

srcrip commented Feb 26, 2024

@josevalim, would it be feasible to add list handling too if these kinds of changes are on the table? I've been doing something like this a little bit as a utility:

  defmodule Utils do
    def get(data, keys)
    def get(data, [h]) when is_function(h), do: h.(:get, data, & &1)
    def get(data, [h | t]) when is_function(h), do: h.(:get, data, &get(&1, t))
    def get(nil, [_]), do: nil
    def get(nil, [_ | t]), do: get(nil, t)

    # list part
    def get(data, [h]) when is_list(data) and is_integer(h), do: Enum.at(data, h)
    def get(data, [h | t]) when is_list(data) and is_integer(h), do: get(Enum.at(data, h), t)

    def get(data, [h]), do: Access.get(data, h)
    def get(data, [h | t]), do: get(Access.get(data, h), t)
  end

@josevalim
Copy link
Member Author

It has been discussed a couple times. Doing list[i] would promote linear access and it is very easy to shoot yourself in the foot. Especially by doing things like put_in list[i], value under a loop. I'd try to find the previous discussions.

We have discussed subset operators, inspired by JSONPath, such as get_in(posts[..].comments[..].author.name) (get the name of all comment authors from all posts) but it comes with other downsides. Definitely a separate convo.

@srcrip
Copy link

srcrip commented Feb 26, 2024

I think putting such access would make the most sense in a helper like get_in though, not dedicated syntax.

Consider the scenario of a user trying to use get_in with a map that contains nested lists. They'll end up decomposing it into a pipeline that calls Enum.at anyways, cause that's what they wanted to do.

@josevalim
Copy link
Member Author

josevalim commented Feb 26, 2024

@srcrip that would be a departure of how *_in functions work though, since none of them change how the brackets are interpreted and rather just delegate.

Your proposed mechanism would not extensible either (because it would be hardcoded as part of get_in) and the rationale of Access is exactly to make the syntax extensible to everyone. What happens if someone wants to provide a similar behavior for, say, Nx.Tensor?

lib/elixir/lib/access.ex Outdated Show resolved Hide resolved
lib/elixir/lib/kernel.ex Outdated Show resolved Hide resolved
josevalim and others added 2 commits February 26, 2024 17:19
Co-authored-by: Leandro Pereira <leandro@leandro.io>
Co-authored-by: Leandro Pereira <leandro@leandro.io>
@josevalim
Copy link
Member Author

Consider the scenario of a user trying to use get_in with a map that contains nested lists. They'll end up decomposing it into a pipeline that calls Enum.at anyways, cause that's what they wanted to do.

Btw, I totally hear you and I agree. However, we need to find a solution that is consistent with the rest of the language. I am not sure at the moment what that is, but let's continue exploring!

@josevalim josevalim merged commit 9dcdc1a into main Feb 27, 2024
17 checks passed
@josevalim josevalim deleted the jv-add-get_in-1 branch February 27, 2024 07:04
@josevalim
Copy link
Member Author

💚 💙 💜 💛 ❤️

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