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.
On Tue, Dec 14, 2021 at 10:25 PM Stefan Chrobot <ste...@chrobot.io> wrote:
> 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()?
>
> wt., 14 gru 2021 o 13:26 Wojtek Mach <woj...@wojtekmach.pl> napisał(a):
>
>> 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
>>
>>
>>
>>
>> On December 14, 2021, "chrobot.io" <ste...@chrobot.io> wrote:
>>
>> 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
>>
>> wtorek, 27 lipca 2021 o 10:02:38 UTC+2 polva...@gmail.com napisał(a):
>>>
>>> The PR has been sent! Link:
>>> https://github.com/elixir-lang/elixir/pull/11149
>>>
>>> Em segunda-feira, 26 de julho de 2021 às 15:07:18 UTC-3, José Valim
>>> escreveu:
>>>
>>>> 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. :)
>>>>
>>>> On Mon, Jul 26, 2021 at 20:01 Paulo Valente <polva...@gmail.com> wrote:
>>>>
>>>>> 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!
>>>>> Em segunda-feira, 26 de julho de 2021 às 12:47:31 UTC-3, Wojtek Mach
>>>>> escreveu:
>>>>>
>>>>>> 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
>>>>>> <https://groups.google.com/d/msgid/elixir-lang-core/9b83a803-c2c1-49e7-a531-56791607bcaan%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>>>> .
>>>>>>
>>>>>> --
>>>>> 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/a96fb19a-8148-41dc-8b45-0e2b9fe755b8n%40googlegroups.com
>>>>> <https://groups.google.com/d/msgid/elixir-lang-core/a96fb19a-8148-41dc-8b45-0e2b9fe755b8n%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>>> .
>>>>>
>>>>
>> --
>> 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-core+unsubscr...@googlegroups.com.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/elixir-lang-core/43f206e4-6b22-4622-b8d9-1ade01556039n%40googlegroups.com
>> <https://groups.google.com/d/msgid/elixir-lang-core/43f206e4-6b22-4622-b8d9-1ade01556039n%40googlegroups.com?utm_medium=email&utm_source=footer>
>> .
>>
>> --
>> 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-core+unsubscr...@googlegroups.com.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/elixir-lang-core/b8beea46f50e401bf00961fff49a76f68c360bb3%40hey.com
>> <https://groups.google.com/d/msgid/elixir-lang-core/b8beea46f50e401bf00961fff49a76f68c360bb3%40hey.com?utm_medium=email&utm_source=footer>
>> .
>>
> --
> 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-core+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/elixir-lang-core/CACzMe7aTSK%3Dpni6gM2xDhXoUKFjJTTZ9cY4TqXniDUtddohA3Q%40mail.gmail.com
> <https://groups.google.com/d/msgid/elixir-lang-core/CACzMe7aTSK%3Dpni6gM2xDhXoUKFjJTTZ9cY4TqXniDUtddohA3Q%40mail.gmail.com?utm_medium=email&utm_source=footer>
> .
>
--
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-core+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4KJq%3DxpXZt713AJCxh25UF%2B2M2Z-%2BhH0Tr1mH0mMwUcVQ%40mail.gmail.com.
