Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

import_predicates_as_macros: included_in?, format?, size? issues #608

Open
aleksandra-stolyar opened this issue Dec 19, 2019 · 9 comments
Open

Comments

@aleksandra-stolyar
Copy link

aleksandra-stolyar commented Dec 19, 2019

class MyContract < Dry::Validation::Contract
  params do
    required(:hogwarts_house).hash do
      required(:head).filled(:string)
      required(:common_room).hash do
        required(:name).filled(:string)
        required(:location).filled(:string)
      end
    end
  end

  rule('hogwarts_house.head').validate(size?: 1..255)
  rule('hogwarts_house.head').validate(format?: SOME_REGEX)
  rule('hogwarts_house.common_room.location').validate(included_in?: ['tower', 'underground'])
end

For included_in? and size? passing an array of integers/strings not working but it works when just one value passed or with predicates which do not require arrays)
included_in?

rule('hogwarts_house.common_room.location').validate(included_in?: ['tower', 'underground'])

size?

rule('hogwarts_house.head').validate(size?: 1..255)
ArgumentError (wrong number of arguments (given 256, expected 2))

This predicate is just returning error.
format?

rule('hogwarts_house.head').validate(format?: SOME_REGEX)
Dry::Container::Error: Nothing registered with the key :format?

@lromeo
Copy link

lromeo commented Dec 30, 2019

Just ran into these same issues with included_in? and format?

@waiting-for-dev
Copy link
Member

#610 adds missing :format? predicate.

About the other two, the issue is that they are related to predicates expecting an array (or range) as argument. It hasn't straightforward solution. Macro argument(s) are forwarded to the predicate method, and we don't have any way to tell whether it expects an array or an unitary item. I see three options:

rule('hogwarts_house.head').validate(size?: [1..255])
rule('hogwarts_house.common_room.location').validate(included_in?: [['tower', 'underground']])
  • Manually keep record of what predicates expect what.

  • Modify dry-logic so that the predicates with array or range arguments also work with splat arguments.

@solnic
Copy link
Member

solnic commented Jan 3, 2020

@waiting-for-dev that's OK, it's always worked like that, going back to dry-validation 0.x. We gotta make sure to document it properly.

@solnic
Copy link
Member

solnic commented Jan 3, 2020

@waiting-for-dev ...no wait, sorry - it did not work like that in dry-validation 0.x! dooh. OK this needs to be investigated because I can't say why it's not working as expected in case of predicate extension :(

@waiting-for-dev
Copy link
Member

waiting-for-dev commented Jan 4, 2020

Currently, a predicate can be of three kinds:

  • #method(input), as in #empty?([1, 2])
  • #method(value, input), as in #gt?(1, 2)
  • #method(list_value, input), as in #included_in?([1, 2], 1)

The last two are problematic when used with import_predicates_as_macros extension. Macro arguments are always collected as an array:

rule(:age).validate(gt: 18) # => `macro.args` is `[18]`
rule(:age).validate(included_in: [18, 21]) # => `macro.args` is `[18, 21]`

In the #included_in? example, of course the user should provide a nested array [[18, 21]], because they want to provide an array argument and not two arguments. But as the predicate method is "hidden", there is the concern whether it means expecting too much from them.

As ruby is a dynamically typed language, there is no way to know whether the expected argument is an array or not. Currently, when arguments are dispatched they are splatted, so when you provide an array like in included_in it results in a wrong number of arguments error.

@waiting-for-dev
Copy link
Member

waiting-for-dev commented Jan 4, 2020

In the case of #size? the user is in fact providing what the predicate method expects, but dry-validation coerces arguments with Array() so the information is lost.

@waiting-for-dev
Copy link
Member

waiting-for-dev commented Jan 4, 2020

Maybe we should:

  • Don't coerce with Array(). Just create a singleton array when provided macro arguments returns false with #is_a?(Array).
  • Document properly the situation for array arguments. Alternatively, change dry-logic to accept splatted arguments, but I don't like moving the complexity to the fundamental piece...

@solnic
Copy link
Member

solnic commented Jan 8, 2020

Don't coerce with Array()

I bet that is the solution we're looking for

@waiting-for-dev
Copy link
Member

The referenced PR should fix the issue here with #size? predicate usage with a range.

About #included_in?, the predicate expects an array so the correct way to specify it is included_in?: [[18, 21]]. Saying [18, 21] would mean that the predicate expected two arguments. I understand this may seem confusing at first sight. As I said, an option would be modifying dry-logic to also accept splatted arguments, but I don't like it very much.

waiting-for-dev added a commit that referenced this issue Jan 15, 2020
waiting-for-dev added a commit that referenced this issue Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants