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

[Ruy_Asan_rubyruy]

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

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

[Ruy_Asan_rubyruy]

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

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

[Ruy_Asan_rubyruy]

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

[Patrick_Aljord]

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

[Michael_Koziarski1 (Michael Koziarski)]

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

Quite

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.

[Rick_DeNatale]

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

Quite

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.

[Ben_Munat]

+1

Ruy Asan (rubyruy) wrote:

[Ruy_Asan_rubyruy]

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

Quite

> 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

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

[Ruy_Asan_rubyruy]

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!

[Michael_Koziarski1 (Michael Koziarski)]

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

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

[Rupert_Voelcker]

<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:

Programmer Insecurity @ iBanjo

[Ruy_Asan_rubyruy]

> 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

> 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.

[Ruy_Asan_rubyruy]

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.

[Ruy_Asan_rubyruy]

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

[Michael_Koziarski1 (Michael Koziarski)]

Also, my MBP is on the fritz again which is going to slow me down substantially while Apple takes care of it (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