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 all errors and fallback to defaults #44

Closed axelson closed 4 years ago

axelson commented 4 years ago

Is it within scope for NimbleOptions to return all errors and fallback to any specified defaults for fields that have an error? Alternatively if it was possible to get all the failing errors (instead of just the first) it would be reasonable to add the fallback logic in my application code.

Also right now it would be difficult to do that fallback logic because the key is not returned as an atom, only the error message is returned within the %NimbleOptionsVendored.ValidationError{}.

josevalim commented 4 years ago

We discussed adding something that returns all errors so :+1:. Not sure about continuing even in face of errors though, what to do when a required field is not given?

Also right now it would be difficult to do that fallback logic because the key is not returned as an atom, only the error message is returned within the %NimbleOptionsVendored.ValidationError{}.

We can totally add more information to the error.

zachdaniel commented 4 years ago

Perhaps something like a fallback configuration would be more expressive than just using the default if the option fails to validate.

Something like:

[
  retries: [
    type: :integer,
    default: 1,
    fallback: 1
  ]
]

Then if your options were [retries: "ten"] it would fall back to 1 instead.

Looking at that though, I can't imagine a scenario where the fallback and default would be different, so perhaps something like:

[
  retries: [
    type: :integer,
    default: 1,
    fallback_to_default: true
  ]
]

The question with that is whether or not "falling back to defaults" is something you want the schema to control, or the caller. It might be preferable for the caller to say NimbleOptions.validate(opts, schema, fallback_to_defaults: true)

Just some ideas :)

whatyouhide commented 4 years ago

To me, the "fallback" idea doesn't really make sense in the context of validation. I would like nimble_options to stay simple, and it seems like a pretty nastily custom use case to have an option that can be invalid but would still be accepted... For example, would you log that you did that? I think it doesn't fall in the responsibilities of nimble_options. Ideally, I'd split this issue in two because I am 👍 too on returning all errors, but I am 👎 on the fallback, so I think the discussions could happen separately?

zachdaniel commented 4 years ago

We can definitely split the issue up :) no need to conflate the two. There are ways to get fallback behavior with custom types also. I disagree with the idea that it's entirely out of the scope of this library, as it seems like another form of validation to me. But that's just my 2 cents.

josevalim commented 4 years ago

If custom types work then that would definitely be my suggestion.

zachdaniel commented 4 years ago

If validate_type was exposed as public, then people could easily write their own higher level custom types this way:

[
  field: [
    type: {:custom, MyMod, :fallback, [:integer, 10]},
    default: 10
  ]
]

def fallback(value, type, default) do
  case NimbleOptions.validate_type(type, value) do
    :ok -> {:ok, value}
    {:error, _} -> default
  end
end
axelson commented 4 years ago

I'm agreed about splitting the discussion on fallbacks out from returning all errors in a more programmatic way. I brought them up together since my overall goal was fallbacks (with programmatic errors being returned as well, not just logging), and that was very difficult to achieve with the current behavior of the library.

And I would definitely respect the opinion that fallbacks are out of scope for this library because there's multiple different behaviors that could be supported or expected, and as I understand it this library is attempting to remain small and focused.

Although I do think that the library should be able to support building fallback options on top of the library without needing to use custom types.

josevalim commented 4 years ago

In any case, PRs for returning all errors and for adding more informations to errors are welcome.

axelson commented 4 years ago

Split the two first portions of this into #45 and 46. Fallback can be discussed after those are complete or just handled in client code once those two features are done.