Closed marten closed 10 years ago
I agree on the reasoning on validate. I think that's a good way to implement that. As said in the other pullrequest, I think the semantics of authorized? returning boolean is better.
I think this can be merged now.
The build is failing though?
This is a rough first spike.
The user must end her
valid?
method witherrors.empty?
or otherwise a false value will be returned if the last check was fine (errors.add(:title, 'must mention pavlov') unless title =~ /pavlov/
returns nil if the title mentions Pavlov).Therefore, it would be better if the user implemented some other method, and
valid?
would be a library method that simply returnserrors.empty?
Thatvalid?
method should probably be public now, since we want to check validity before calling operations.A sensible name for the method that the user should implement would be
validate
, but that's already taken by a method that would possibly be better namedcheck_validation
(since it's similar tocheck_authorization
).The names of various methods could ideally be cleaned up, but that's hard to do backwards-compatibly. For consistent naming, the user should have to implement
validate
,authorize
,execute
. But that means swapping the meaning ofvalidate
andvalid?
with respect to the current way.But replacing
def valid?
withdef validate
might not be such a big deal.In summary: I'd love some feedback on how we could do this while maintaining a smooth upgrade path.