[Proposal] Include Keyword default argument validation

[Paulo Valente]

One common problem we face when dealing with keyword list opts is validating options passed and setting default values to said keyword.

This commonly yields code like:

def f(x, opts \\ []) do

opt_1 = opts[:opt_1] || :default_1
opt_2 = opts[:opt_2] || :default_2
...
end

Nx.Defn.Kernel has an useful helper for these cases (https://github.com/elixir-nx/nx/blob/c8353de695a3d70dc6d518e0ab2ec832faa7df68/nx/lib/nx/defn/kernel.ex#L741).

Perhaps this helper could be included in the Keyword module, along with a non-rasing version of some kind?

[José Valim]

I would propose to add this to the Kernel module, to mirror the struct! helper. But adding to Keyword also works. Thoughts?

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/09f8df11-459a-44a6-aaf2-34462069d8ccn%40googlegroups.com.

[Stefan Chrobot]

This would be really useful. Two things to consider.

Should the bang version really raise for redundant options?. If foo(opts) wraps bar(opts) with extra options, foo() needs to clean up the opts before passing them down which is some of the most annoying plumbing code out there. Also, if a function removes an option, the error is only reported at runtime. It's a breaking change in terms of the function's API, but I'm not a fan of the raise in this case. Maybe a non-bang version is the solution here.

What do you think about supporting pattern matching? %{opt1: opt1, opt2: opt2} = <something>!(opts, [:opt1, :opt2]) looks pretty good if you need to access multiple options or use an option multiple times. Would be nice to be able to do this without the need for Enum.into() or Enum.sort(). Not sure what "something" should be - maybe something opts specific ("opts!" maybe?).

Seems like my concerns can be addressed with Keyword.merge() and Map.merge(), but streamlining this common use case would be great.

Best,

Stefan

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4%2B4tjxfOhvZ2ky%3Dw-u%2Bpfe0xmWEhGRQBxZS8a0fYzgeTQ%40mail.gmail.com.

[José Valim]

 

Should the bang version really raise for redundant options?. If foo(opts) wraps bar(opts) with extra options, foo() needs to clean up the opts before passing them down [...] Maybe a non-bang version is the solution here.

Correct. But I'd say that's what I would want from a bang function. I know this will lead to stricter APIs in some cases but those changes can be very helpful in some scenarios. For example, in Nx some APIs receive axis (singular) and other axes (plural) and raising on unknown options is important here. We could support a non-bang variant but it wouldn't be different from Keyword.merge/2 - so I am not sure of the value it gives besides consistency?

 

What do you think about supporting pattern matching?

If you need a Map, then calling Map.new afterwards is the way to go (and most likely the most efficient implementation).

[Paulo Valente]

> I would propose to add this to the Kernel module, to mirror the struct! helper. But adding to Keyword also works. Thoughts?

Makes sense! I suggested Keyword mostly because there's the module already, but since we have struct! in Kernel, it might be better to have keyword! there too. Usage would also be nicer this way.

> Should the bang version really raise for redundant options?

As José said, it would help clean out stray options being passed. I can think of a few cases in which I thought one option was being used, but had either a typo or wrong name altogether (think "end" vs "end_datetime" sort of situation).
However, I can see a less strict version being useful as well. Perhaps there could be a keyword!/3 which accepted options like: keyword!(opts, [extra_keys: true], key1: :default1, key2: :default2)

[José Valim]

I debated this with the Elixir team and we agreed on the following API:

    Keyword.validate! :: keyword | no_return

    Keyword.validate :: {:ok, keyword} | {:error, unknown_keys, missing_keys}

The same functionality would have to be defined for Map.

The API for Keyword.validate! and Map.validate! would be the exact same as the one found in Nx.Defn.Kernel.keyword!.

Could you please send a PR? Or open up an issue if you can't submit it at the moment.

Thanks!

[Paulo Valente]

Sure! I'll try to send a PR soon!

Thanks :)

[Wojtek Mach]

Keyword.validate will accept just a keyword, right? Did you consider making it accept any enumerable of pairs, just like Keyword.new does? Same for Map.

I think one particular scenario is something like this:

    def foo(opts) when is_list(opts) do
      config = Map.validate!(opts, …)
      config.name # the assertive map.key is really convenient here

what do you think? The only concern, besides feature creep :), is perhaps validate is not the best name given it accepts broader set of inputs. But then again, Keyword.keyword and in particular Map.map are maybe a bit awkward. 

On July 26, 2021, josevalim <jose....@gmail.com> wrote:

I debated this with the Elixir team and we agreed on the following API:
    Keyword.validate! :: keyword | no_return    Keyword.validate :: {:ok, keyword} | {:error, unknown_keys, missing_keys}
The same functionality would have to be defined for Map.
The API for Keyword.validate! and Map.validate! would be the exact same as the one found in Nx.Defn.Kernel.keyword!.
Could you please send a PR? Or open up an issue if you can't submit it at the moment. 

Thanks!On Monday, July 12, 2021 at 12:46:38 PM UTC+2 polva...@gmail.com wrote:

> I would propose to add this to the Kernel module, to mirror the struct! helper. But adding to Keyword also works. Thoughts?

Makes sense! I suggested Keyword mostly because there's the module already, but since we have struct! in Kernel, it might be better to have keyword! there too. Usage would also be nicer this way.

> Should the bang version really raise for redundant options?

As José said, it would help clean out stray options being passed. I can think of a few cases in which I thought one option was being used, but had either a typo or wrong name altogether (think "end" vs "end_datetime" sort of situation).
However, I can see a less strict version being useful as well. Perhaps there could be a keyword!/3 which accepted options like: keyword!(opts, [extra_keys: true], key1: :default1, key2: :default2)

 --
 You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
 To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/9b83a803-c2c1-49e7-a531-56791607bcaan%40googlegroups.com.

[Paulo Valente]

I was considering making it only accept keywords, and return keywords as well.

However, I can see benefits in accepting Enumerables. What do you think about using another verb such as cast or coerce, instead of validate, for this typing
(e.g. @spec Keyword.cast!(Enumerable.t(), keyword) :: keyword | no_return and @spec Map.cast!(Enumerable.t(), keyword) :: map | no_return)?

Should I also implement it for Map as well, or maybe add it in a second PR?

Thanks for the idea on Enumerables!

[José Valim]

In this case, I would call Keyword.validate! And then call Map.new. I don’t see a benefit in the generic API as it won’t be faster and not necessarily clearer either. So I would stick with my last proposal. :)

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/a96fb19a-8148-41dc-8b45-0e2b9fe755b8n%40googlegroups.com.

[Paulo Valente]

The PR has been sent! Link: https://github.com/elixir-lang/elixir/pull/11149

[Stefan Chrobot]

Hi, I'd like to follow up on this since Keyword.validate has landed in 1.13. Is there a will to use the validation in common libraries?

Just recently I've been hit by misspelling "redact: true" with "redacted: true" when defining a field in Ecto. Sounds like a perfect use case for Keyword.validate. Ideally this would explode, but that's a breaking change, so emitting a warning would probably make more sense.

The other thing is that most libraries tend to stay on the oldest Elixir version possible, so they can't just call Keyword.validate (unless I'm missing something). In our case we're running on Elixir 1.10, but we backport small bits because of their usefulness (so for example we have Future.Keyword.validate and Future.tap). Is something like that a viable way to do this in libraries? I guess the other option is conditional compilation, but it sounds like a code that's going to stay there for a very long time (what would be the incentive to bump the minimum version as high as 1.13?).

Looking forward to your thoughts on this.

Best,

Stefan

[Wojtek Mach]

I don't think most features should be backported as it increases maintenance burden (on features themselves but also more minor stuff like @doc since). I definitely agree about the usefulness though. I tend to vendor-in the new functionality and at compile-time pick between the vendored and upstream implementation. For Keyword.validate/2 it would be:

    # TODO: use Keyword.validate when we require Elixir 1.13
    if Version.compare(System.version(), ">= 1.13.0") do
      defp keyword_validate(keyword, values) do
        Keyword.validate(keyword, values)
      end
    else
      defp keyword_validate(keyword, values) when is_list(keyword) and is_list(values) do
        validate(keyword, values, [], [], [])
      end

      defp validate([{key, _} = pair | keyword], values1, values2, acc, bad_keys)
           when is_atom(key) do
        case find_key!(key, values1, values2) do
          {values1, values2} ->
            validate(keyword, values1, values2, [pair | acc], bad_keys)

          :error ->
            case find_key!(key, values2, values1) do
              {values1, values2} ->
                validate(keyword, values1, values2, [pair | acc], bad_keys)

              :error ->
                validate(keyword, values1, values2, acc, [key | bad_keys])
            end
        end
      end

      defp validate([], values1, values2, acc, []) do
        {:ok, move_pairs!(values1, move_pairs!(values2, acc))}
      end

      defp validate([], _values1, _values2, _acc, bad_keys) do
        {:error, bad_keys}
      end

      defp validate([pair | _], _values1, _values2, _acc, []) do
        raise ArgumentError,
              "expected a keyword list as first argument, got invalid entry: #{inspect(pair)}"
      end

      defp find_key!(key, [key | rest], acc), do: {rest, acc}
      defp find_key!(key, [{key, _} | rest], acc), do: {rest, acc}
      defp find_key!(key, [head | tail], acc), do: find_key!(key, tail, [head | acc])
      defp find_key!(_key, [], _acc), do: :error

      defp move_pairs!([key | rest], acc) when is_atom(key),
        do: move_pairs!(rest, acc)

      defp move_pairs!([{key, _} = pair | rest], acc) when is_atom(key),
        do: move_pairs!(rest, [pair | acc])

      defp move_pairs!([], acc),
        do: acc

      defp move_pairs!([other | _], _) do
        raise ArgumentError,
              "expected the second argument to be a list of atoms or tuples, got: #{inspect(other)}"
      end

      defp keyword_validate!(keyword, values) do
        case keyword_validate(keyword, values) do
          {:ok, kw} ->
            kw

          {:error, invalid_keys} ->
            keys =
              for value <- values,
                  do: if(is_atom(value), do: value, else: elem(value, 0))

            raise ArgumentError,
                  "unknown keys #{inspect(invalid_keys)} in #{inspect(keyword)}, the allowed keys are: #{inspect(keys)}"
        end
      end
    end

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/43f206e4-6b22-4622-b8d9-1ade01556039n%40googlegroups.com.

[Stefan Chrobot]

When would it make sense to add validation for options using Keyword.validate (or the backported variant) in Elixir's standard lib, Ecto or Phoenix?

There's a runtime penalty for doing the validation. Would it make sense to add it "everywhere" but actually do it only for certain values of Mix.env()?

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/b8beea46f50e401bf00961fff49a76f68c360bb3%40hey.com.

[José Valim]

Projects should not be accessing Mix.env() at runtime though, as Mix.env() is not even available inside releases.

Also note that adding Keyword.validate/2 to existing APIs can be a backwards incompatible change, because people may be passing a bunch of unrelated options. We probably need to add some guidelines to the function documentation, but I think that focusing mostly on "DSLs" is a good starting point.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CACzMe7aTSK%3Dpni6gM2xDhXoUKFjJTTZ9cY4TqXniDUtddohA3Q%40mail.gmail.com.