-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
User defined validations #71
Conversation
@@ -7,6 +7,11 @@ | |||
require 'lotus/validations/attribute' | |||
require 'lotus/validations/errors' | |||
|
|||
begin | |||
require 'byebug' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this debug
Done |
include Lotus::Validations | ||
attr_accessor :foo | ||
|
||
validates :foo, :is_bar do |attribute| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a syntax which made using a self defined validator more straight-forward.
Something like:
class BarValidator
def self.call(attribute)
attribute.value == 'bar'
end
end
validates :foo, :is_bar, with: BarValidator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. But in my opinion a validator should handle adding the validation error by itself:
class BarValidator
def self.call(attribute, errors)
errors.add(attribute.name, Error.new(@name, name, result, @value)) unless attribute.value == 'bar'
end
end
validates :foo, with: BarValidator
Maybe there could be a class for validators which provide a an interface to simplify adding errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like that idea as well. In my apps I sometimes use different Error classes and pattern match over them. This makes it easier to serialize certain types of errors. So yes, adding your own error class seems wise to me.
@kbredemeier Hello and thanks for this PR. A block that yields only the attribute isn't enough. Sometimes you need values from other attributes (think of "password confirmation" case). I like this syntax: It's better to design a |
@kbredemeier @ianks That being said, would you folks love to contribute, think and code a solution for this? That would be AWESOME!! Ref #72 |
I like the idea too, IMO you have to worry about message error in your validator but not instantiate a class inside. |
# @api private | ||
def user_defined | ||
@validations.each do |name, validation| | ||
break unless validation.respond_to? :call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this to blow up instead of silently ignore the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @pascalbetz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that is that @validations
may not only contain user defined validations. The build in vaildations are also in this hash and they are not callable objects (If I remember right).
I will look into this. While I am doing this I would also like to extract the build-in validations from the |
I'm very much for this idea. It makes more sense to have them decoupled from the attribute class. I would just make sure to check that they are not significantly less performant.
In this case, would the validator check if something falsey is returned? If so, it would be nice to be able to specify an error message as well. |
|
||
def validation_name | ||
@validation_name ||= begin | ||
class_name = self.class.name.split('::').last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kbredemeier Please use Lotus::Utils::String#underscore
for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought there must be something like this. Coming from rails and didn't know yet about Lotus::Utils
. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why passing the attributes hash and not the entity (or any ruby object which responds to attribute_name ?
* add_error now takes the expected value and an hash with optional arguments * renamed Lotus::Validations::Validation#validation_name to #default_validation_name * added documentation to Lotus::Validations::Validation
I think I am happy with the interface of the |
Looks good. The only thing I am not keen on is the add_error method. I think using a keyword arg for expected would be nice, so it is not ambiguous as to the meaning. Thoughts? |
…the expected value as a keyword arg
You are right. Passing the expected value as a keyword argument makes it much more expressive. |
Hi guys, I'm experimenting with custom validations in an Hanami app and I'd like to share some thoughts of mine.
So, these all said, I've been playing with something like this to define custom validations for Hanami: To declare custom validations we could use any of these: validates :location, with: LocationValidator
# May be accept also an instance? That way we could parametrize validations, for instance
validates :location, with: RegexValidator.new(/.../, validation_name: :location_format)
# or to avoid singleton or dup problems
validates :location, with: proc { RegexValidator.new(/.../, validation_name: :location_format) }
validates :some_value, custom: :even_number do |validation|
validation.add_error unless validation.actual_value.to_i % 2 == 0
end Validator objects (such as LocationValidator and RegexValidator) and blocks should understand Here's an actual (partial) implementation that I'm playing with: class LocationValidator
include Validation
validation_name :existent_location
# Validating
def validate()
add_error unless is_location_valid?
end
# Asking
def is_location_valid?()
Google::Geocoding.query_address(actual_value).addresses_count == 1
end
end module Validation
# Class methods
def self.included(base_class)
base_class.class_eval do
extend ClassMethods
end
end
module ClassMethods
def validation_name(validation_name)
@validation_name = validation_name
end
def get_validation_name()
@validation_name
end
def validate(attributes, errors, attribute_name)
self.new.call( ValidationContext.new(attributes, errors, attribute_name) )
end
end
# Instance methods
# Validating
def call(validation_context)
@validation_context = validation_context
@validation_context.set_validation_name(self.class.get_validation_name) unless self.class.get_validation_name.nil?
validate
end
def validate()
raise 'Implementor class responsibility'
end
# Accessing
def validation_context()
@validation_context
end
def attributes()
validation_context.attributes
end
def errors()
validation_context.errors
end
def attribute_name()
validation_context.attribute_name
end
def validation_name()
validation_context.validation_name
end
def set_validation_name(validation_name)
validation_context.set_validation_name(validation_name)
self
end
def actual_value()
validation_context.actual_value
end
def namespace()
validation_context.namespace
end
def actual_value_of(attribute_name)
validation_context.actual_value_of(attribute_name)
end
# Adding errors
def add_error(attribute_name: nil, validation_name: nil, expected_value: nil, actual_value: nil, namespace: nil)
validation_context.add_error(attribute_name: attribute_name, validation_name: validation_name, expected_value: expected_value, actual_value: actual_value, namespace: namespace)
end
def add_error_for(attribute_name, validation_name: nil, expected_value: nil, actual_value: nil, namespace: nil)
validation_context.add_error_for(attribute_name, validation_name: validation_name, expected_value: expected_value, actual_value: actual_value, namespace: namespace)
end
def add_validation_error(validation_error)
validation_context.add_validation_error(validation_error)
end
# Running other validations
# Run the validation named validation_name on the attribute named attribute_name
def validate_attribute(attribute_name, with: validation_name)
validation_context.validate_attribute(attribute_name, with: with)
end
# Answer whether there was an error from the validation named validation_name on an attribute
def validation_failed_for?(:validation_name, on: nil)
validation_context.validation_failed_for?(:validation_name, on: on)
end
end class ValidationContext
# Initializing
def initialize(attributes, errors, attribute_name = nil, validation_name = nil)
@attributes = attributes
@errors = errors
@fully_qualified_attribute_name = attribute_name
@validation_name = validation_name
end
# Accessing
def attributes()
@attributes
end
def errors()
@errors
end
def fully_qualified_attribute_name()
@fully_qualified_attribute_name
end
def set_validation_name(validation_name)
@validation_name = validation_name
end
def validation_name()
@validation_name
end
def attribute_name()
fully_qualified_attribute_name.split('.')[-1]
end
def namespace()
fully_qualified_attribute_name.split('.')[0..-2].join('.')
end
# Querying
def actual_value()
actual_value_of(fully_qualified_attribute_name)
end
# Answer the value of the attribute named attribute_name
def actual_value_of(attribute_name)
attribute_name.split('.').inject(attributes) { |value, accessor|
value[accessor]
}
end
# Adding errors
def add_error(attribute_name: nil, validation_name: nil, expected_value: nil, actual_value: nil, namespace: nil)
attribute_name = self.attribute_name if attribute_name.nil?
validation_name = self.validation_name if validation_name.nil?
expected_value = true if validation_name.nil?
actual_value = self.actual_value if actual_value.nil?
namespace = self.namespace if namespace.nil?
add_error_for(
attribute_name,
validation_name: validation_name,
expected_value: expected_value,
actual_value: actual_value,
namespace: namespace
)
end
def add_error_for(attribute_name, validation_name: nil, expected_value: nil, actual_value: nil, namespace: nil)
add_validation_error(
Hanami::Validations::Error.new(
attribute_name,
validation_name,
expected_value,
actual_value,
namespace
)
)
end
def add_validation_error(validation_error)
errors.add(validation_error.attribute, validation_error)
end
# Running other validations
# Run the validation named validation_name on the attribute named attribute_name
def validate_attribute(attribute_name, with: validation_name)
# ...
end
# Answer whether there was an error from the validation named validation_name on an attribute
def validation_failed_for?(:validation_name, on: nil)
attribute_name = on.nil? : fully_qualified_attribute_name : on
! errors.for(attribute_name).empty?
end
end Conditional validations can be handled like this: validates :some_value, custom: :even_number do |validation|
unless actual_value.nil?
validation.add_error unless validation.actual_value.to_i % 2 == 0
end
end or it would be even better if we could invoke and ask the result of another validations: class LocationValidator
include Validation
validation_name :existent_location
# Validating
def validate()
return if validation_failed_for?(:presence)
return if validation_failed_for?(:location_format)
add_error unless is_location_valid?
end
# Asking
def is_location_valid?()
Google::Geocoding.query_address(actual_value).addresses_count == 1
end
end Any thoughts? |
This feature was implemented by #100 https://github.com/hanami/validations#custom-predicates |
I would like to define my own validations like this:
This pull request includes a first implementation and a integration test to illustrate the wanted behavior. Feedback welcome