Include argument names in AST

[fran-worley]

1) predicate.args should be a hash with arg_names as keys and arg_values as values. 2) predicate ast should be a nested array including all arguments, their names and values.

[fran-worley]

This is an initial look at adding arg_names into the ast. It's not exactly beautiful but it works...

Rule#to_ast and Predicate#to_ast are no longer used by Result when generating the final ast this is because the complete predicate ast includes call args which can't be stored in the predicate itself.

I've left the methods largely as we use them in specs but are they really necessary? The only benefit I can see is that they define a rule before it has been called. (and dry-validation uses them) so they are definitely useful.

Predicate#call now returns a PredicateResult object, not true/false. You will now need to call result on the PredicateResult object to get true/false.

I'm not sure how much of a problem this causes, but I couldn't find a way around it.

[fran-worley]

Requiring argument names in the ast causes a number of issues in dry-validation.

1. Macros:
The ast gets generated with just an array of argument values here: https://github.com/dry-rb/dry-validation/blob/master/lib/dry/validation/schema/value.rb#L114

And I'm struggling to see a good way of getting the argument names here. Short of loading the the predicate and calling to_ast.

2. Message tokens: I had hoped to be able to use the predicate argument names as the names of tokens in error messages. This would give you an easy way to define variables for error messages both in static predicates and in custom ones. However this doesn't work with examples like included_in? and excluded_from? which require the list as a comma separated string and not an array. At present we can't perform transformations in error messages. Would it be a good idea to be able to do something like this?

size?:
  arg:
    default: "size must be %{size}"
     range: "size must be within %{size.first} - %{size.last}"
included_in?: "must be one of: %{list.join(',')}"

[solnic]

I planned to make an improvement in the dsl part of dry-v so that when we're generating rule ast we also already have access to predicate registry so that we can raise errors when invalid predicates are being defined. When you have a predicate registry, you also have access to arg names, so we could build an ast including names.

Caveat: a schema object (not its class) has predicates too, and we invoke the DSL before a schema instance is created, so it's a chicken-egg issue. We could look into create a predicate registry for the DSL by merging configured Predicates module (class-level access, so we have it before a schema object is created) and somehow extract predicate methods from a schema.

One solution would be to actually change how accessing predicates on a schema object works. Currently we have Schema#[] which can return an existing predicate if it's in the configured class-level registry, or see if a schema implements that predicate, then it returns a Dry::Logic::Predicate instance and pass in method object as the predicate function. Now, the problem here is that we don't have these predicates when DSL is invoked, so now I'm thinking about changing it so that we actually create unbound predicates using method_added hook and bind them when creating a schema instance and passing that registry, with all schema predicates bound to its instance, to the rule compiler.

There are actually nice benefits of doing so, first of all we would have an explicit representation of schema-bound predicates, and we could easily avoid schema rule recompilation when using Schema#with and only recompile rules that have predicates that must be bound to a schema.

So yeah, I feel like this is the way to go with this. It's gonna be a really nice improvement.

[fran-worley]

Thanks for the thoughts @solnic. I'll raise a PR in dry-validation and copy these notes over.

I'll also run the branch against the specs for dry-types and see what breaks. -> Now done and they all pass.

[solnic]

@fran-worley if you're interested in improving how we handle predicates in dry-v, I'd suggest doing that as a first step, before integrating with the extended dry-logic ast

[fran-worley]

Now that is a good idea @solnic. I'll try and have a look today.

[solnic]

closing in favor of #17