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

Validations-as-classes was inspired (loosley) by the Validatable gem.

A validation class can turn itself into a macro (using
Base.define_validation_macro(base)) based on it's name.

e.g.
ValidatesPresenceOf.define_validation_macro(ActiveRecord::Base)
# now we can do
class ActiveRecord::Base
validates_presence_of #etc - as you're used to
end

(This is done automatically for the pre-defined validations of course
- but it's bitch-easy to implement your own and add them where you
need them.)

The macro will instantiate the validation class and add it to a
base.validations hash, which stores validations by attribute name. (So
validates_presence_of :name, :password will generate TWO
ValidatesPresenceOf instances, one in base.validations[:name] and one
in base.validations[:password])

This alone has advantages: we can finally attempt to serialize
validations for public consumption elsewhere. Auto-generated
JavaScript validation is one obvious application, converting some
validations to RDBMS constraints is a more exotic one.

Validations::Base contains several facilities for declaring valid
options, marking some as required, validating their type etc.

For the most part, subclasses need only declare their options using
the above helpers, and implement a valid? instance method.

A perfectly simple example:
http://github.com/rubyruy/rails/tree/recursive_errors/activemodel/lib/active_model/validatable/validations/acceptance.rb

A more complicated one:
http://github.com/rubyruy/rails/tree/recursive_errors/activemodel/lib/active_model/validatable/validations/length.rb


The Base class also takes care of common options, such
as :allow_nil, :allow_blank, :if and so forth (and also provides a
convenient 'hook' point for adding new 'global' options, such as :on
for ActiveRecord).

TODO:
:if/:unless are not actually supported yet - I would like to implement
them using a callback (:should_run) - which could be used to implement
not only :if/:unless but also :allow_nil/:allow_blank and
Validatable's (the gem) :group option.

Unfortunately there is no way to pass additional arguments to
callbacks (like the object instance... kinda important) using
AciveSupport::Callbacks, so I will probably have to either add that to
AS:C or start hammering out ActiveModel::Callbacks.


ActiveModel::Validatable::Errors
================================

My changes to errors are the most drastic departure from the existing
ActiveRecord code.

The first 'design goal' is to make Errors more 'familiar' with ruby
data types.

One thing that (IMO) causes a lot of confusion (in a general sense) is
how .errors[:whatever] can sometimes return an array of strings, and
sometimes just the string. NOTHING ELSE EVER DOES THIS IN RUBY. EVER.

It also makes it a PITA to work with errors since you need to check
the return type and UGH - it's just so NOT GOOD.

So basically, I'm putting my foot down and saying: Erros is basically
an Array. I would have it inherit from Array, but I'm not smart enough
to do that in a good way with the current implementation (there are
some special considerations, described below). No doubt, somebody else
will be.

Actually there's all sorts of things that are pretty 'evil' in the
current implementation and will greatly benefit from some attention
from the community.

Some basic examples:

@person.errors[0] # => "Name can't be blank."
@person.errors.first #=> "Name can't be blank."
@person.errors.each # .. etc, it all works as expected

For all you care, it's an Array! Ok? Ok!

The second 'design goal' is to encapsulate error messages in a class
of their own. More on this later, (under
ActiveModel::Validatable::ErrorMessages).

# This does however mean you shouldn't do this:
@person.errors << "Failure!"
# You could do this instead:
@person.errors << ErrorMessage.new(...)
# But really, you should just do this:
@person.errors.add "Failure"

The add() method constructs an appropriate ErrorMessage object. I
could have << behave the same way, but I worry about the implications
for more complicated mutation methods like merge or somesuch.

We'll look more in depth at adding errors later.

So other then add(), how else is it not like an Array?
Well you'll want to be able to filter errors by attribute. The Array-
ness of Errors is more important to me then it's Error-ness, so
@person.errors[:name] is no longer supported. Instead we just have the
old @person.errors.on(:name).

And what does that return? Why another Errors (Array) of course!

@person.errors[0] # => "Person is too ugly."
@person.errors[1] # => "Name can't be blank."
@person.errors[2] # => "Address can't be blank."

@person.errors.on(:name)[0] # => "Name can't be blank"
@person.errors.on(:address)[0] # => "Address can't be blank"

@person.errors.on(:name).class # => ActiveModel::Validatable::Errors

# What about the first one though?
@person.errors.on(:base)[0] # => "Person is too ugly"
@person.errors.on(:base).size # => 1
# Yay awsome!


One neat thing about this is that if the attribute whose errors you
want has a .errors of it's own, they can be included recursively:

@person.errors[0] # => "Name can't be blank."
@person.errors[1] # => "Company name is taken."

@person.on(:name)[0] # => "Name can't be blank."
@person.errors.on(:company)[0] # => "Company name is taken."
@person.errors.on(:company).on(:name)[0] # => "Company name is
taken."

@person.errors.on(:company) == @person.company.errors # Not exactly
true, but that's the idea.

Now one problem you might already have noticed is that when you ADD an
error to an Errors array, some confusion arises as to where to
actually store the error. I hope the behaviour is *mostly* intuitive,
but there are definitely a few confusing edge cases.

LEAKY ABSTRACTION WARNING: One key thought to hang on to is that
READING from an Errors object works on a different underlying array
then WRITING to the same Errors object.

Examples:

@person.errors.on(:name).add "Name is stupid!"
@person.errors.on(:title).add "REALLY now? A title?"

@person.errors == ["Name is stupid!","REALLY now? A title?"]
@person.errors.on(:name) == ["Name is stupid!"]
@person.errors.on(:title) == ["REALLY now? A title?"]

# So far so good yes?

@person.errors.add "I hate this person."
# ... is actually the same as ...
@person.errors.on(:base).add "I hate this person."

@person.errors == ["I hate this person.", "Name is stupid!","REALLY
now? A title?"]
# (note that :base errors always show up first)

# Now for something REALLY confusing!
@person.errors.on(:company).add "This person can't work at this
company."

@person.errors == ["I hate this person.", "Name is stupid!","REALLY
now? A title?", "This person can't work at this company."]
@person.errors.on(:company) == ["This person can't work at this
company."]
# HOWEVER!
@person.company.errors == []
# ZUH?

Rationale:
When you go @errors.on(:company) we assume you mean there is a problem
with this particular association, NOT the company as a whole.
The comapny is 'valid', it's just that this person working there is
not a valid situation.

# To add an error to the company itself, you can (predictably I
hope) do this:
@person.company.errors.add "Company is not a legal employer."

@person.errors.on(:company) == ["This person can't work at this
company.","Company is not a legal employer."]
@person.company.errors == ["Company is not a legal employer."]

That's not so bad now is it? It also deals with adding errors to an
aggregate association (@person.errors.on(:friends).add) quite
elegantly...

Do note however, that if you were to dig deeper, you DO end up
modifying the associate object's errors:

@person.errors.on(:company).on(:name).add "Company name sounds
silly."

@person.errors.on(:company) == ["This person can't work at this
company.","Company is not a legal employer.","Company name sounds
silly."]
@person.company.errors == ["Company is not a legal
employer.","Company name sounds silly."]
@person.company.errors.on(:name) == ["Company name sounds silly."]
@person.errors.on(:company).on(:name) == ["Company name sounds
silly."]

Seems reasonable that if the error is specific to an attribute of the
association it's not particular to just this association.
Also, the alternative would be a pain to implement. ;)


So yes, there are some complications here, but I think they are mostly
isolated to pretty useless / ridiculous edge cases.

Overall, Errors should be much easier to work with (due to their array-
likeness) and we can think of .on() as a simple filter for this array.
You should also keep in mind that manually adding errors is a MUCH
less common case then reading from them, and most of the confusing
bits from this API only apply to adding errors.

ActiveRecord::Validatable::ErrorMessage
=======================================

There are two reasons for making this a class:

1. Generating well worded error messages which include data only
known at validation time, such as
the name of the field we're validating, the validated value, and
validation config options (like max-length.)
- e.g. "{attribute_name} must be between {min} and {max} long."
- e.g. "{attribute_name} '{value}' is already taken by
{other_user}"
- a MUST for internationalizable default messages (think word
order problems)
Having this behavior be isolated in a class gives us a lot more
power and control over the error message template.

2. Allow the errors messages to be more self contained, which also
makes them more 'portable'.
This means we don't need to have an error's +Errors+ object to
know what attribute it applies to.
We can also do neat tricks ilke send back the error template as
JSON with {attribute_name} left unrepalced
so that it can be populated from the *actual* HTML LABEL element
via JS (and it ALSO gives us a clue as to where
to which INPUT the error belongs to). We are already doing this
in our production app and it's VERY handy!

Basically, an ErrorMessage is a template string. Each ErrorMessage
knows what object and attribute it's for(if any), and what validation
generated it (if any).

Other then that - it IS a string (it inherits from String) - and the
'inner string' is the compiled message template. The compilation is
eager - as soon as the ErrorMessage is instantiated.

The template format is pretty straight-forward "It looks like this,
and some values are {replaced}" and "{replaced}" gets replaced with
the return value of a method named :replaced.

Basically it will try (in sequence):
validation.replaced
self.replaced
value.replaced
object.replaced

Two important methods for self (i.e. the ErrorMessage itself) are
attribute_name (the humanized attribute name to which this error
message applies) and value (the attribute's value, which failed
validation).

As such, default validations are complete sentences. "can't be empty"
becomes "{attribute_name} can't be empty." (PS: Error messages are
sentences. Sentences end in periods (or some other form of
punctuation). Let's raise the standard for default error messages.)



Ok so that's the bird's eye view. Look through the source (
http://github.com/rubyruy/rails/tree/recursive_errors ) for details.
Tests, as I mentioned, aren't complete, but I have a decent start on
them, so feel free to look through those too.

PATCHES (better yet, pull requests) WELCOME!!

(comments are good too)

[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
==============

- no longer callback chain based

- every validation is an object
- one validation object per class, per attribute
- but can also just attach validations to another object instead
- like ohhh... saaay... a state machine transition object
- BAM - instant state-based validations
- cool possibilities:
- serialize validations to JS functions (or somesuch)
- translate certain validation to RDBMS constraints
- the point is, it's easier/better to reason about validation
as stand-alone abstractions

- common validation base class is super handy
- easy to add options across validations (:unless, :groups -
whatever you feel like)
- allows us to keep AM:Validations context-neutral
- ActiveRecord-specific options like :on are easily added
without disturbing AM:V

2. Errors
=========

- Previous implementation filled me with hate:
- .errors[:whatever] #=> maybe an array, maybe just 1 item
- WTF who does this? nobody.
- Not quite a hash, not quite anything else
- HATE

- New implementation: ERRORS ARE AN ARRAY
- @person.errors -> returns array-like of ALL errors,
including
:base, errors on associates, whatever
- @person.errors[:name] is a no-no. [] is used for index access
just like a normal array. e.g. @person.errors[2..4] etc

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

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

Read mega-post above for the details.

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

--
Cheers

Koz

[Rick DeNatale]

On Thu, Jul 10, 2008 at 3:40 AM, Michael Koziarski <mic...@koziarski.com> wrote:
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.

--
Rick DeNatale

My blog on Ruby
http://talklikeaduck.denhaven2.com/

[Ben Munat]

+1

[Ruy Asan (rubyruy)]

On Jul 10, 12:40 am, "Michael Koziarski" <mich...@koziarski.com>
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.

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)]

On Jul 10, 6:34 am, "Rick DeNatale" <rick.denat...@gmail.com> wrote:
> 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 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

> Then again I'm still pretty new at this so there's a good chance I'm
> missing something ;)
> >
>

--
Cheers

Koz

[Rupert Voelcker]

2008/7/11 Michael Koziarski <mic...@koziarski.com>:
<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

[Ruy Asan (rubyruy)]

On Jul 11, 3:15 am, "Michael Koziarski" <mich...@koziarski.com> wrote:
> > 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)]

On Jul 12, 1:10 am, "Rupert Voelcker" <rup...@rupespad.com> wrote:
> 2008/7/11 Michael Koziarski <mich...@koziarski.com>:

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 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 ;)

--
Cheers

Koz