search

dashbitco / nimble_options

A tiny library for validating and documenting high-level options. 💽
Apache License 2.0
507 stars 38 forks source link

Distinguish duplicated keys in error message #84

Closed fishtreesugar closed 2 years ago

fishtreesugar commented 2 years ago

fixes https://github.com/dashbitco/nimble_options/issues/83

whatyouhide commented 2 years ago

Should we treat these as separate errors? I don't think duplicate keys is as common as unknown options, so maybe two different errors would be nice.

fishtreesugar commented 2 years ago

make sense, and updated.

whatyouhide commented 2 years ago

The code looks good. Now I’m thinking about this more though, and I’m wondering: should we actually raise on duplicated keys, or should we just pick the first value and validate that? Keyword lists can generally have duplicates and a "common" way of overriding a key is to do something like [{:my_key, ...} | keyword_list]. Thoughts @josevalim?

josevalim commented 2 years ago

Keeping the first is also a good idea!

whatyouhide commented 2 years ago

After all, if you know that you don't want duplicate options, you can always do a quick pass before calling to nimble_options that essentially raises if keys != Enum.uniq(keys).

whatyouhide commented 2 years ago

After a bit more thought, I do think that duplicate keys can be handled before feeding options into nimble_options, so for now I'll close out this PR. If we'll want to introduce this behavior in the future, I think it should be opt-in, so maybe through an option:

NimbleOptions.validate(opts, schema, allow_duplicates: false)

Right now we don't even have validate/3 though, so we might want to think of a few more use cases for options before we go this route. Thank you so much for your work anyways @fishtreesugar, and for the interesting discussion! 💟