dashbitco / nimble_options

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

Feature request: return programmatic errors #46

Closed axelson closed 4 years ago

axelson commented 4 years ago

Broken off from #44

whatyouhide commented 4 years ago

@axelson Iā€™m not sure about what this issue is. #44 talked about 1. returning all errors and 2. having a fallback. Not sure what you mean by programmatic errors? :bowtie:

axelson commented 4 years ago

Having a fallback is my eventual goal, but it seemed unlikely to be accepted into nimble_options. So including "programmatic" errors means that the errors would have all the information needed to implement fallback in client code without having to parse the error message string (which would be very brittle).

Here's an initial thought for what the error value could be:

iex> schema = [
  age: [type: :pos_integer, default: 1],
  name: [
    type: :keyword_list,
    keys: [
      first: [type: :string, default: ""],
      last: [type: :string, default: ""]
    ]
  ]
]
iex> opts = [age: "42", name: [first: :bob, last: :johansson]]
iex> NimbleOptions.validate(opts, schema)
{:error,
[
  %ValidationError{
    key: [:age],
    message: <message>
  },
  %ValidationError{
    key: [:name, :first],
    message: <message>
  },
  %ValidationError{
    key: [:name, :last],
    message: <message>
  }
]

So then the client code would know that age, first name, and last name are invalid, and it could call NimbleOptions with an updated opts, doing a fallback or other logic. Currently the client code has no idea which field(s) has failed and is unable to do any type of fallback logic on its own. Does that make it clearer?

(Note: before making the PR I plan to review how ecto returns its errors, and check if it would make sense for the return value to look closer to that)

whatyouhide commented 4 years ago

@axelson we already return what you call "programmatic errors" since we return NimbleOptions.ValidationError. Either I'm missing something or this issue is basically closeable :)

axelson commented 4 years ago

@whatyouhide Right now supporting a fallback would require very brittle code. Take this example schema and result:

iex> schema = [dialyzer_format: [type: :string, default: "dialyxir_long"], auto_fetch_deps: [type: :boolean, default: true]]
iex> opts = [dialyzer_format: 123, auto_fetch_deps: false]
iex> NimbleOptions.validate(opts, schema)
{:error,
 %NimbleOptions.ValidationError{
   keys_path: [],
   message: "expected :dialyzer_format to be an string, got: 123"
 }}

In order to properly fallback to dialyzer_format = "dialyxir_long" we would need code like:

iex> {:error, %{message: message}} = NimbleOptions.validate([dialyzer_format: 123, auto_fetch_deps: false], schema)
iex> [[_, key_str]] = Regex.scan(~r/expected :([a-zA-Z_]*) to be/, message)
iex> bad_key = String.to_existing_atom(key_str)
iex> new_opts = Keyword.delete(opts, bad_key)
iex> NimbleOptions.validate(new_opts, schema)
{:ok, [dialyzer_format: "dialyxir_long", auto_fetch_deps: false]}

The step I want to avoid with this issue is the Regex.scan along with String.to_existing_atom which is why the proposal returns the failing key as an atom. And similarly for nested keys the path is required.

Does that make sense?

Edit: initially sent early

whatyouhide commented 4 years ago

We can freely add metadata to the error without breaking, absolutely. We can add two new fields to NimbleOptions.ValidationError:

Does that make sense? cc @josevalim

josevalim commented 4 years ago

Sure.

whatyouhide commented 4 years ago

@axelson can you send a PR for this change? Thanks šŸ’Ÿ

axelson commented 4 years ago

Sent #48!