ActiveModel::Validatable(Total Rewrite, Totally Awesome) - Request for comment on work in progress

BEHOLD: http://github.com/rubyruy/rails/tree/recursive_errors

I didn't really want to "come out" with this just yet - although I'm
95% certain this is the general architecture I want to go with, I'm
only about 60% or so done with the implementation, and only 40% or so
done with the testing.

I also haven't actually started running backwards compatibility tests,
mostly because I hadn't really settled on what I would be deprecating
(and of course some stuff just plain isn't done yet so I'd get lots of
failures about crap I already know doesn't work).

I have added comments (that look like # RUY-NOTE) at the top of most
source-files to indicate what is mostly done, and what is still in
progress, and what is marked for death. I have also added detailed
overviews for the main classes - but I will be pasting most of them in
this post for your reading pleasure.

Regardless, I mentioned my work in passing on #rails-contrib today and
it turns out technoweenie has been working on more or less the same
thing, but since we've both been publishing-shy we totally missed each
other and now there is overlap. Go go "release early" I guess :frowning:

Anyway, so here we are - I'm "releasing" it - but do keep in mind this
is pre-alpha. Implementation is NOT complete.

I'm looking for feedback (and of course patches! but my expectations
are low), specifically about:

1. Are the public-facing API changes too extreme? (Validations
themselves are mostly the same, Errors is quite different however, but
IMO, well worth it)

2. Does the current "design" (architecture? whatever) make it easier
to implement some neat validation feature you've been burning to add?
I tried to pick up as many ideas as I could from "the wild" and pretty
much everything I saw should be very very easy to add (if I haven't
added them already).

Some examples include: internationalizable default error messages, JS
validations generated from model the model class, reporting errors via
JSON, per-state or per-transition validations with state machine,
Validatable's :group option.

3. What color should the bike-shed be?

Ok so what exactly is this anyway?

Well, it's my take on ActiveModel::Validations, which will hopefully
replace ActiveRecord::Validations one day soon, and of course it will
also form the basis for ActiveResource::Validations and could in
theory be used for any number of things (like presenter models,
couchdb models, whatever).

The most obvious changes it the rename to Validatable (mostly because
i wanted to save Validations for the modules containing the validation
classes itself)

Major changes:
  1. Errors
    - fairly different .errors API
    - error messages themselves encapsulated in ErrorMessage class
  2. Validations are now classes

Also note that because this module MUST be able to stand alone (for
use in presenters etc), it does not COMPLETELY supplant
ActiveRecord::Validations. AR-specific stuff like
the :on=>:save|:update|:create option, as well as
validates_uniqueness_of are taken care of by AR:V, albeit by extending
AM:V

I will go over the new validations class first because it's less
contentious and generally speaking more awesome.

ActiveModel::Validatable::Validations::Base

It has come to my attention the above wall of text is a little
intimidating and is holding back adoption :wink:

So I present to you a summary, in ADD-friendly, fixed-with 68-char
column formatted (to appease google's crazy ML reader) point form:

- I re-wrote ActiveModel::Validations -> renamed to
  ActiveMoldel::Validatable
  - Inspired by the Validatable gem (sort of)
  - Would like to replace ActiveRecord::Validation with it

MAJOR CHANGES:

1. Validations

Oh and I completed the callbacks-with-argument thing - see here:
http://rails.lighthouseapp.com/projects/8994/tickets/589-args-option-for-run_callbacks

:if/:unless work now

looks good to me, but why so much hate? =P

It has come to my attention the above wall of text is a little
intimidating and is holding back adoption :wink:

Quite :wink:

So I present to you a summary, in ADD-friendly, fixed-with 68-char
column formatted (to appease google's crazy ML reader) point form:

- I re-wrote ActiveModel::Validations -> renamed to
ActiveMoldel::Validatable
- Inspired by the Validatable gem (sort of)
- Would like to replace ActiveRecord::Validation with it

Rick's the guy to talk with about this stuff, he's been more vocal
about about changing active record's validations to be based on active
model's.

- @person.errors[:name] is a no-no. [] is used for index access
   just like a normal array. e.g. @person.errors[2..4] etc

I definitely don't like the maybe-return-an-array thing, but
errors[:foo] is fine. I don't quite follow why you haven't made it an
alias for on or something similar. errors[2] seems more yagni, who
wants to have error number 3?

Alternatively, the [] method can just do different things if it's
passed a symbol or an int / range.

- You can FILTER errors for a specific attribute using
errors.on()
- e.g. @person.errors.on(:name)
- or maybe @person.errors.on(:company)
   - includes errors from associated Company object
- the returned array is still an Errors instance
   - so you can do
     @person.errors.on(:company).on(:business_number)

nice

- Error messages themselves encapsulated in ErrorMessage class
- each error message knows who it belongs to
- handy for certain kinds of AJAX tricks
- easier to work with in general
- @person.errors.add("msg") converts strings to ErrorMessages
   automatically
- @person.errors.on(:name).add("msg") will do the right thing
   for configuring the ErrorMessage object
- lots of edge cases -> see bigger writeup above

- ErrorMessages have far more powerful templating facilities then
the printf based system from before.
- e.g. "{attribute_name} '{value}' must not exceed {max}
   {units}."
   - shows up as "User name 'fofofofofoffofofo' must not exceed
     10 characters."
- More helpful default error messages, no longer constrained in
   word-order.
   - Pretty much required for i18n-able default error messages.

That's the gist of it!

Nice work.

If I can make a process related comment though, it would have been
cool to bounce some of these ideas around here first before dropping
such a large patch. You could have got a few people to help out and
got feed back on the API before you had code and tests that'd need
changing.

Without attributing Ruy Asan who wrote:

It has come to my attention the above wall of text is a little

intimidating and is holding back adoption :wink:

Quite :wink:

So I present to you a summary, in ADD-friendly, fixed-with 68-char

column formatted (to appease google’s crazy ML reader) point form:

  • I re-wrote ActiveModel::Validations -> renamed to

ActiveMoldel::Validatable

  • Inspired by the Validatable gem (sort of)
  • Would like to replace ActiveRecord::Validation with it

Rick’s the guy to talk with about this stuff, he’s been more vocal

about about changing active record’s validations to be based on active

model’s.

That’s the gist of it!

Nice work.

If I can make a process related comment though, it would have been

cool to bounce some of these ideas around here first before dropping

such a large patch. You could have got a few people to help out and

got feed back on the API before you had code and tests that’d need

changing.

In any event, I’m glad to see this come up. Not having looked at the actually code in the patch, this looks like it might help address my current pet peeve about rails validations which is that the walkbacks you get when a validation throws an exception are pretty much worthless for determining WHICH validation failed.

The app I work on for a living is complex and has lots of validations and observers. I’ve had to table moving to Rails 2.1 because it now seems to run validations more often, and debugging the resultant chaos was taking more time than it was worth. If the new implementation leaves more clues on the invocation stack in these cases, I’d be all for it.

Another benefit of what I’m reading is that it would make validations reflectable which would help in writing TDD/BDD assertions/matchers that a model did or did not validate certain things.

+1

Ruy Asan (rubyruy) wrote:

> It has come to my attention the above wall of text is a little
> intimidating and is holding back adoption :wink:

Quite :wink:

> So I present to you a summary, in ADD-friendly, fixed-with 68-char
> column formatted (to appease google's crazy ML reader) point form:

> - I re-wrote ActiveModel::Validations -> renamed to
> ActiveMoldel::Validatable
> - Inspired by the Validatable gem (sort of)
> - Would like to replace ActiveRecord::Validation with it

Rick's the guy to talk with about this stuff, he's been more vocal
about about changing active record's validations to be based on active
model's.

I've been talking to him throughout this on #rails-contrib

> - @person.errors[:name] is a no-no. [] is used for index access
> just like a normal array. e.g. @person.errors[2..4] etc

I definitely don't like the maybe-return-an-array thing, but
errors[:foo] is fine. I don't quite follow why you haven't made it an
alias for on or something similar. errors[2] seems more yagni, who
wants to have error number 3?

Alternatively, the [] method can just do different things if it's
passed a symbol or an int / range.

It's mostly a philosophical thing: if it looks like an array it should
really act like an array as much as possible. Least surprise and all
that. Having symbol indexes strongly suggests a Hash-like rather then
an Array-like which I'm trying to move away from.

I'm 100% for keeping errors[:foo] in a deprecation layer however.

If I can make a process related comment though, it would have been
cool to bounce some of these ideas around here first before dropping
such a large patch. You could have got a few people to help out and
got feed back on the API before you had code and tests that'd need
changing.

Well I did do some bouncing, but only on IRC. I was afraid that
describing my ideas in words would be as time consuming in english as
it would in code, if not more. I'm still not convinced this isn't the
case :wink:

I'm reasonably confident in the code's and tests' flexibility however
so feedback is still very much welcome!

I suppose i did miss out on getting people to be more directly
involved since it's less interesting to be involved in somebody else's
baby if you weren't there from the start, but I dunno - I haven't
really seen much evidence of large-scale cooperative patches in OSS
first hand.

Then again I'm still pretty new at this so there's a good chance I'm
missing something :wink:

In any event, I'm glad to see this come up. Not having looked at the
actually code in the patch, this looks like it might help address my current
pet peeve about rails validations which is that the walkbacks you get when a
validation throws an exception are pretty much worthless for determining
WHICH validation failed.

That depends, what exactly made the backtraces suck before? I'm
guessing methods generated by Callbacks, but those should still show
the right file/line number (iirc). If not, then that's a separate fix
to ActiveSupport::Callbacks (and a simple one at that).

Although validations are no longer BASED on callbacks they still use
callbacks for conditional validation.

Another benefit of what I'm reading is that it would make validations
reflectable which would help in writing TDD/BDD assertions/matchers that a
model did or did not validate certain things.

ABSOLUTELY it would! :slight_smile:

It's mostly a philosophical thing: if it looks like an array it should
really act like an array as much as possible. Least surprise and all
that. Having symbol indexes strongly suggests a Hash-like rather then
an Array-like which I'm trying to move away from.

I'm 100% for keeping errors[:foo] in a deprecation layer however.

I don't think I buy that reasoning, there's no useful reason to ask
for a numeric offset into the collection of errors on a given object.
By contrast there's a very obvious case to ask for the errors on a
given field.

I'm not sure I follow why you want it to be array like?

Well I did do some bouncing, but only on IRC. I was afraid that
describing my ideas in words would be as time consuming in english as
it would in code, if not more. I'm still not convinced this isn't the
case :wink:

I'm reasonably confident in the code's and tests' flexibility however
so feedback is still very much welcome!

I suppose i did miss out on getting people to be more directly
involved since it's less interesting to be involved in somebody else's
baby if you weren't there from the start, but I dunno - I haven't
really seen much evidence of large-scale cooperative patches in OSS
first hand.

The reason you don't see many cases of this is that, unfortunately,
large 'patchbombs' rarely end up getting applied. We have a large
body of tests and users which we need to keep satisfied unless there's
a very very good reason not to. By starting over rather than making
lots of incremental changes you've left yourself in a bit of a
difficult situation, the implementation may have been easier but
getting it applied is going to be much much harder because of the
testing and backwards compatibility hurdles you're going to have to
overcome.

Rick already has a branch where he's moving all the validations into
active model, you should work with him to combine your design
improvements with his branch, that'll make it much easier to get the
code back into core.

http://github.com/technoweenie/rails/commits/validations

<snip>

The reason you don't see many cases of this is that, unfortunately,
large 'patchbombs' rarely end up getting applied. We have a large
body of tests and users which we need to keep satisfied unless there's
a very very good reason not to. By starting over rather than making
lots of incremental changes you've left yourself in a bit of a
difficult situation.

<snip>

There's an interesting blog post on this sort of issue:

http://blog.red-bean.com/sussman/?p=96

> It's mostly a philosophical thing: if it looks like an array it should
> really act like an array as much as possible. Least surprise and all
> that. Having symbol indexes strongly suggests a Hash-like rather then
> an Array-like which I'm trying to move away from.

> I'm 100% for keeping errors[:foo] in a deprecation layer however.

I don't think I buy that reasoning, there's no useful reason to ask
for a numeric offset into the collection of errors on a given object.
By contrast there's a very obvious case to ask for the errors on a
given field.

I'm not sure I follow why you want it to be array like?

The main reason I want them to be array-like is that there is no
compelling reason to make them hash-like, but there are some reasons
against it, specifically, the fact that hashes can't make guarantees
about order.

Basically, when you encounter symbol-keys inside a [] you would expect
to be working with a Hash, or Hash-like, which has implications, like
order guarantees, the number of arguments .each{} takes and so forth.

In my opinion, errors.on(:foo) is not so much worse then errors[:foo]
that it's worth this little extra bit of confusion.

Or put another way: errors[:foo] just doesn't support the principle of
least surprise.

Of course, I'll also admit that since errors really ISN'T an array
anyway, some unpleasant surprises are unavoidable.

Finally, it just isn't a big deal so whatever - the bike-shed can be
whatever color is most popular.

> Well I did do some bouncing, but only on IRC. I was afraid that
> describing my ideas in words would be as time consuming in english as
> it would in code, if not more. I'm still not convinced this isn't the
> case :wink:

> I'm reasonably confident in the code's and tests' flexibility however
> so feedback is still very much welcome!

> I suppose i did miss out on getting people to be more directly
> involved since it's less interesting to be involved in somebody else's
> baby if you weren't there from the start, but I dunno - I haven't
> really seen much evidence of large-scale cooperative patches in OSS
> first hand.

The reason you don't see many cases of this is that, unfortunately,
large 'patchbombs' rarely end up getting applied. We have a large
body of tests and users which we need to keep satisfied unless there's
a very very good reason not to. By starting over rather than making
lots of incremental changes you've left yourself in a bit of a
difficult situation, the implementation may have been easier but
getting it applied is going to be much much harder because of the
testing and backwards compatibility hurdles you're going to have to
overcome.

The branch should eventually pass all existing tests so I'm hoping
that will significantly alleviate things.

I could also attempt to split-up the Errors part and have the included
first, although it strikes me an awkward operation.

But hey, if it helps, I'll do it.

Rick already has a branch where he's moving all the validations into
active model, you should work with him to combine your design
improvements with his branch, that'll make it much easier to get the
code back into core.

http://github.com/technoweenie/rails/commits/validations

Yes that would be ideal.

Well, to my defense, this is more about being able to illustrate my
thoughts better in working code and unit tests rather then some other
kind of description.

Like I said, I had been bringing up ideas on IRC for a while, and I
went over small-ish changes first, but the feeling I was getting was
that a) people prefer seeing code rather then just hear ideas and
suggestions and b) nobody was particularly happy with the old
validations implementation so the option of a total re-write was on
the table.

So here we are.

Again, I really want to emphasize that I am open to changes, including
nuking large swaths of what I already implemented.

Also, my MBP is on the fritz again which is going to slow me down
substantially while Apple takes care of it :frowning: (my only backup computer
runs Windows)

Also, my MBP is on the fritz again which is going to slow me down
substantially while Apple takes care of it :frowning: (my only backup computer
runs Windows)

Ouch, my macbook is flickering and I'm terrified at the same prospect.

Working with rick's branch definitely sounds like the right way to
move this stuff forward, then he can simply merge it when he feels
it's ready. Whatever ends up coming out of that branch you've got
some good improvements there. Especially the ErrorMessage
abstraction. Very nice.

Great work on the first 90% of the work, good luck with the second 90%
you have left :wink: