team-formalist / formalist-rb

Flexible form builder
MIT License
22 stars 4 forks source link

[WIP] Require a dry-validation schema, and offer validated/unvalidated results #21

Closed timriley closed 8 years ago

timriley commented 8 years ago

:hand: :warning: :construction: :hand:

This requires every form to have a corresponding dry-validation schema. The ultimate goal is to remove any type coercion work from formalist itself and rely on dry-validation's own type coercions.

It also changes the API to better support the two distinct use cases you have with a form:

  1. Preparing a form with sane, initial input from your database and sending it to the view for rendering.

    my_form = MyForm.new(my_schema)
    my_form.build(input) # returns a `Formalist::Form::Result`
  2. Then, receiving the data from the posted form, coercing it and validating it (and potentially re-displaying the form with errors):

    my_form = MyForm.new(my_schema)
    my_form.receive(input).validate # returns a `Formalist::Form::ValidatedResult`

The main differences are as such:

So far, all of the above is in place and working.

What's left to do is:

timriley commented 8 years ago

@makenosound Here's my thinking about removing the type information from the field AST elements:

Firstly, by requiring each form to be provided a corresponding dry-validation schema, we're already getting type coercion via the schema, so that's the first reason to drop the type specification from the fields.

The second reason was that it was looking like a field's particular type was already implied by a lot of the field "display modes" we offer, and also given that HTML forms end up being all string-y no matter how much you try. However, I realise we do have some overlap, particularly around e.g. "radio" and "select" display modes. And I do realise that we're doing a lot of form manipulation in JS here, where the difference between a string and an int actually has some meaning.

So unless you think, based on some of my thinking above, that it would actually be reasonable to drop type information from the field AST, I'll make sure I keep it around. What I suspect will change will be that you don't declare the type explicitly yourself when defining the fields – formalist will look up the type from the dry-v schema and use that automatically.

Let me know what you think :)

makenosound commented 8 years ago

So unless you think, based on some of my thinking above, that it would actually be reasonable to drop type information from the field AST.

I’ve been thinking about it a bit more today and I still don’t really think it’s reasonable. Having a good UI for each types depends on, in places, explicitly knowing about that type. In those instances you’d need to pass through the type anyway, and I think it’s conceptual simpler for them to always have one.

What I suspect will change will be that you don't declare the type explicitly yourself when defining the fields – formalist will look up the type from the dry-v schema and use that automatically.

Yep, that seems totally reasonable to me.

timriley commented 8 years ago

@makenosound Just FYI, I've updated the to-do list above to make sure we pull the type information from the schema and include that in the AST like we do now.

The only thing that's not clear is how we might handle arrays of primitives. I'll need to look into how that might be encapsulated by a schema.

timriley commented 8 years ago

I'm going to merge the current state of this branch into master, since it already provides a really helpful improvement to the current API. It'll let us handle input processing and validation in a much more straightforward way in our initial few projects using formalist.

However, I'll keep this PR open as a place to implement the rest of the schema-integration items in the to-do list at the top of this page.

timriley commented 8 years ago

We should be able to infer arrays of primitives from schemas like this:

class MySchema < Dry::Validation::Schema::Form
  key(:ages) { |ages| ages.array? { ages.each(&:int?) } }
end
timriley commented 8 years ago

I'm closing this now. The work we did here was merged into master, we've since changed direction somewhat, and a lot of the goals here are now being achieved in #24.