add Access.pop/3 to support a specific default value

[tudborg]

Adds Access.pop/3 which allows specifying a default value for pop (like Map.pop/3 and Keyword.pop/3)

Currently I've implemented it completely separate from Access.pop/2,
to avoid compatibility issues with pop/2.

So the underlying container must also implement Access.pop/3 for this to work.
For supporting containers, they'd probably just implement a

def pop(container, key, default \\ nil)

which would take care of pop/2 and pop/3.

But I'm wondering if there might be a better way to handle this such that pop/2 is just an inlined pop(container, key, nil).

[josevalim]

@tudborg yes, in this case, the container would have to implement it, so this effectively a change of contract from pop/2 to pop/3. I believe originally we had reasons to not go with pop/3 but I can't recall them at the moment. It may be related to how pop_in works and the fact it can abort early too.

[tudborg]

Yea, I was looking at using pop_in and Access.key/2 with a default, but Access.key/2 only works on maps and structs, and I need to accept maps and keyword lists.

In my specific case, the performance of the pop doesn't really matter, and I don't need nested access.
If Access.key/2 would work for keyword lists as well, Access.pop/3's use-case would almost go away I guess.

My data isn't nested, so what I do today to emulate pop/3 is something like

def my_pop(container, key, default \\ nil) do
  case Access.fetch(container, key) do
    {:ok, value} ->
      {_also_value, updated_container} = Access.pop(container, key)
      {value, updated_container}
    :error ->
      default
  end
end

But an

pop_in(container, [Access.key(key, default)])

would work for me, if Access.key would work with Keyword lists.

@josevalim Is there a reason for Access.key not supporting keyword lists?

Would that be a better solution?

[josevalim]

I believe I'd indeed prefer to change something like Access.key but I am almost sure the selectors in Access are meant to be type precise instead of polymorphic. One option is to add Access.keyword(...), but we can add keywords to Access.key. Thoughts?

[tudborg]

That's a possibility.
Although that wouldn't solve my specific issue of having a polymorphic key-able container.

I personally don't think container-type specific accessors are very useful.
If I knew my container was a Keyword list, I'd just use the Keyword module.

The better ergonomics of get_in might be a reason to do it maybe.

Anyway.

Access.key is map-specific, but isn't that a incidental because structs are maps, so it technically works for both without additional work. And if that is the case, why not Keyword lists?

It feels reasonable to me that Access.key would return an accessor that is capable of accessing a "key" in any container that is "key"-able.
At least that feels like the most intuitive interpretation to me.

But I also don't know the reason for adding the accessor functions, so the intended use-case might be lost to me.

Originally I did not consider the Kernel functions that use Access.
But I see that they are pretty tightly coupled.

I feel like adding Keyword list support to Access.key is reasonable.
If you pass anything but a map today, the container is still passed to Map.* and you get a BadMapError.
But I'm sure there are code out there relies on that BadMapError 😬

[josevalim]

We can support keywords in key if you need polymorphism. And perhaps introduce map_key and keyword_key for those who want explicitly non polymorphic versions. A PR is welcome!

[sabiwara]

I am almost sure the selectors in Access are meant to be type precise instead of polymorphic

I agree.

but we can add keywords to Access.key

Wouldn't this make it harder from the type system to infer the actual accepted type?

Another idea could be to add Access.default(key, default) which could work with any key and any compatible type - but not structs that don't implement Access, just like container[key] || default (it's not like we need to pop keys on structs)

[josevalim]

I don’t think the type system should be a primary concern here. We have many polymorphic interfaces and that’s fine. If they are not, the time to revisit them is yet to come. It is not even clear if the type system can infer nested access in general. That’s a bridge to cross later. :)

[tudborg]

@sabiwara That would work as well!

I like Access.default/2.

{value, rest} = pop_in(container, [Access.default(key, default)])

For my specific use-case Access.pop/3 would be cleaner

{value, rest} = Access.pop(container, key, default)

But I get why that might not be desirable, because it forces the container to implement a pop/3 to avoid breaking backwards compat (or at least make it slower, because we'd have to check for an exported pop/3), and that might be confusing.

I still think Access.key could support keyword lists though.
But if there are strong opinions on that, default would work too.

WDYT?

[sabiwara]

I think both work and can be useful - after all we have the possibility to add a default for map keys but not for other containers, feels like a gap/inconsistency 🙂

[josevalim]

I find Access.default kind of inconsistent with the other functions and it doesn't tell anything about the type of argument we are expecting. :(

[tudborg]

Alright. 👍

Let me throw together a PR for an Access.key that works for keyword lists and see where that takes us.