dashbitco / nimble_options

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

Add what validation failed to `NimbleOptions.ValidationError` #131

Open anthonator opened 2 months ago

anthonator commented 2 months ago

I'd like to be able to perform some additional processing when validation fails for specific reasons. This is difficult to do considering there's no way to know what caused the validation error beyond the error message.

I could see a :validation field on NimbleOptions.ValidationError that would have a value of :required, {:type, :string}, etc.

whatyouhide commented 2 months ago

Okay, to do this we would need a possibly significant overhaul of how we represent errors internally in nimble_options.

Right now, errors are free-form strings. This can be problematic for things like #132 for example, because the string is unstructured. Instead we could:

This is a poor implementation of exceptions and Exception.message/1 maybe, but I don't want to create an exception module for each possible error type πŸ˜‰

If you want, you could give this implementation a tryβ€”and ask any questions if you have them. However, I’m not so sure it will make sense in nimble_options as it has to keep one property: nimbleness πŸ˜„ I would like to maintain this lib on the smaller side.

@josevalim btw any thoughts on this? I’m open to the idea above because I think it could make the lib easier to maintain and contribute to in the future (plus, types!).

josevalim commented 2 months ago

It feels we could tackle this problem by adding some metadata to the ValidationError struct without changing the error message? The only issue is that validation errors can be nested (for example, a nested keyword, and tracking this whole "path" may be complicated - although still doable with a list).

whatyouhide commented 2 months ago

Adding metadata to the error struct and leaving the error message alone is another valid approach yeah but I think it would end up being more work than turning the reason into a structured type and then converting that into a message when needed?

josevalim commented 2 months ago

Another option is to allow them to be MFAs or {&fun/1, metadata}, and then folks can match on the metadata bits they find interesting.

whatyouhide commented 2 months ago

Ah yeah you are correct that the predefined error reason (:required, or :mismatching_type, or whatever) are only the built-in ones but we also have totally custom validation.

So I think we have two possible directions:

  1. We go with the "types" option. This is gonna work great with the type system πŸ˜„ This option would mean something like: errors become structured. If we make these exceptions, then users can extend them by returning {:error, Exception.t()} from validation functions and that way we can always turn anything into a message. I love this.
  2. We go the opposite direction: we keep the messages as strings everywhere but we introduce the concept of free-form metadata everywhere. So we add metadata (:required and friends), users can use {:custom, mod, fun, args, metadata} as a type, or even have :validation_metadata as a top-level key in the nimble_options schema.

Preferences?

josevalim commented 2 months ago

@anthonator which kind of validations do you want to know about? If it was required or a type violation?

@whatyouhide Are people supposed to use NimbleOptions for anything else? In an ideal world, this project is dead once the type system lands (either because it is not needed or because it is part of core).

anthonator commented 2 months ago

2. We go the opposite direction: we keep the messages as strings everywhere but we introduce the concept of free-form metadata everywhere. So we add metadata (:required and friends), users can use {:custom, mod, fun, args, metadata} as a type, or even have :validation_metadata as a top-level key in the nimble_options schema.

The freeform metadata idea was something I was actually going to suggest in the issue description. Even with the {:custom, mod, fun, args, metadata} type. I was envisioning a :private (or something to that effect) field on NimbleOptions.ValidationError for this. Wish I had now πŸ˜„ Regardless, with two people coming to this conclusion independently it seems like it might be a good approach.

Regarding 1, I'm not exactly sure what you mean by "type system". I'm not sure if you mean the new Elixir type system or maybe something you've been discussing internally for NimbleOptions. So I can't really comment on that approach. However, you seem excited about it and that kind of gets me excited about it πŸ˜„

anthonator commented 2 months ago

@josevalim for my specific use case I really just want to know if the error was due to :required or :type. If it was :type then what the option's type is as well.

It might also be useful to add a :type field to NimbleOptions.ValidationError for use beyond this feature as well. If that makes sense to you I can open a new issue to discuss.

josevalim commented 2 months ago

That sounds straight-forward to me and probably how I would tackle this (i.e. solving this specific problem), but once again, it gets trickier once we consider nesting.

anthonator commented 2 months ago

Why does this feature need to worry about nesting given that NimbleOptions.ValidationError has keys_path which tracks where in the nested options the error occurred.

josevalim commented 2 months ago

I forgot about keys path. So adding a field that has value of either :required or {:type, type} is good enough to me.

whatyouhide commented 2 months ago

Agreed, let's solve the specific problem for now without going nuts. πŸ˜„

@anthonator wanna make a PR?

anthonator commented 2 months ago

Heck yeah!

I won't be able to start on this until after #133 is resolved.