Proposal for convention regarding custom validator

[ericproulx]

I would like to propose an update on how custom validator are registered within Grape. Actually, its based on an inherited hook that computes a short name based on the class's name. It works great but I think that setting a convention would simplify the process and the overall understanding of custom validators.

Basically, I'm suggesting that custom validator should be declared the same way has the built-in ones:

• Declared In Grape::Validations::Validators modules
• Class name must end with Validator

That way, instead of relying on the inherited hook, we could just call Grape::Validations::Validators.const_get(computed_short_name) when compiling. We're already using this pattern for versioning and coercing

It's not explicitly said in our documentation but right now anyone can add custom versioners and coercers by following the convention. In addition, parsers, formatters and error_formatters could also benefit from a convention and ease customization. In lower versions, we had Grape::Util::Registrable but I removed it not long ago since it wasn't really documented.

In any case, I've created a PR that applies Grape's convention and surprisingly its 🟢. That means that we don't have any tests that tries to load a real custom validator. I'll check that out.

Thanks everyone.

[ericproulx]

Well, our custom validator in the test suite are following the convention, that's why its 🟢

[dblock]

I think it's odd that we would force users into a namespace that they don't control. Why should MyCode be adding anything to Grape's namespace with the risk of accidentally monkey-patching something?

I guess my main question is, what is the advantage? In what way would it simplify writing a custom validator to the user?

PS: It does look like we are missing a spec for the example in README, which you should add regardless so that we at least know what behavior we would be breaking.

[ericproulx]

Custom validators might already monkey-patch the built-in ones even though its outside Grape's namespace. When you look at how the registration works, it falls in the inherited hook and any code base that has a custom validator named Length are probably overriding it without knowing it. They will notice it at runtime since its probably not relying on the same parameters and it crashes.

Running the spec/grape/validations/validators/length_validator_spec.rb with a custom Length validator at the top overrides the Grape::Validations::Validators::LengthValidator.

class Length < Grape::Validations::Validators::Base
  def validate_param!(attr, value)
    raise ArgumentError, 'my length validator'
  end
end

describe Grape::Validations::Validators::LengthValidator do
 ...
end

Now, if custom and built-in validators share the same namespace, it gives you better idea that you might be overriding a built-in one.

I've looked at how Faraday handles custom middleware's registration and maybe we could do something similar with the registration function. Something like

Grape::Validations::Validators.register_validator(my_validator: MyValidator)

At least, the intention is clear and we could check for the overriding part.

[dblock]

I've looked at how Faraday handles custom middleware's registration and maybe we could do something similar with the registration function. Something like

Grape::Validations::Validators.register_validator(my_validator: MyValidator)

At least, the intention is clear and we could check for the overriding part.

Yes, I'd be in favor of something more explicit like that.