Rails core - Aug 2008 - composed_of constructor and converter options patch

Hi,

I''ve created a patch for composed_of here:
http://rails.lighthouseapp.com/projects/8994/tickets/892-composed_of-constructor-and-converter-options

As summarised in the ticket:

1. It adds a new :constructor option that takes a symbol or a proc
that will be used to instantiate the aggregate object. It is optional
and if not used then the existing behaviour of calling new with the
mapped attributes will be used.

2. It deprecates the use of a block to provide a method of converting
values assigned to the aggregate attribute. The use of a block didn’t
feel particularly consistent with the rest of Rails where typically
symbols or procs are passed as options (a good example being the :if
and :unless options for validations). Of course passing a block will
still function, so existing code won’t break, but it will raise a
deprecation warning. This change leads on to…

3. It adds a new :converter option that also takes a symbol or proc
that will be used when the aggregate attribute is assigned a value
that is not an instance of the aggregate class. This replaces the old
block syntax and makes it easier to do things like calling composed_of
from within a with_options block. If both the :converter option and a
block are passed to composed_of then the :converter option will take
precedence.

Feedback or, even better, +1''s appreciated...

Thanks,
Rob
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Kind of on topic: I have a generator available as a plugin to make
building compositions easier. It also provides a base class for
aggregations that make working with them a little less annoying.

You can find it at http://github.com/caring/composition_generator

./script/generate composition address street city state zip_code

class Person < ActiveRecord::Base
  has_address
end

Cheers,
Chris Eppstein

On Aug 25, 10:59 am, rob-twf <rob.ander...@googlemail.com>
wrote:
> Hi,
>
> I''ve created a patch for composed_of
here:http://rails.lighthouseapp.com/projects/8994/tickets/892-composed_of-...
>
> As summarised in the ticket:
>
> 1. It adds a new :constructor option that takes a symbol or a proc
> that will be used to instantiate the aggregate object. It is optional
> and if not used then the existing behaviour of calling new with the
> mapped attributes will be used.
>
> 2. It deprecates the use of a block to provide a method of converting
> values assigned to the aggregate attribute. The use of a block didn’t
> feel particularly consistent with the rest of Rails where typically
> symbols or procs are passed as options (a good example being the :if
> and :unless options for validations). Of course passing a block will
> still function, so existing code won’t break, but it will raise a
> deprecation warning. This change leads on to…
>
> 3. It adds a new :converter option that also takes a symbol or proc
> that will be used when the aggregate attribute is assigned a value
> that is not an instance of the aggregate class. This replaces the old
> block syntax and makes it easier to do things like calling composed_of
> from within a with_options block. If both the :converter option and a
> block are passed to composed_of then the :converter option will take
> precedence.
>
> Feedback or, even better, +1''s appreciated...
>
> Thanks,
> Rob
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Rob,
I like your patch: it solves problems I have had and the aesthetics of
a proc feel right.  But the duplication between the constructor code
and the converter code is not DRY.  Why not just lump them together
into :constructor?  By providing the option to use a proc, you''ve
already addressed the requirement to deal with differing constructor
arguments:

class Visitor
  composed_of :ip_addr,
              :class_name => ''IPAddr'',
              :mapping => %w(ip to_i),
              :constructor => Proc.new { |value| value.is_a?
(Integer) ? IPAddr.new(value, Socket::AF_INET) :
IPAddr.new(value.to_s) }
end

That should DRY things up a bit without sacrificing any functionality.

PS - I''m the author of the patch (that got a "won''t
fix") you
reference in your lighthouse ticket.


On Aug 25, 1:59 pm, rob-twf <rob.ander...@googlemail.com>
wrote:
> Hi,
>
> I''ve created a patch for composed_of
here:http://rails.lighthouseapp.com/projects/8994/tickets/892-composed_of-...
>
> As summarised in the ticket:
>
> 1. It adds a new :constructor option that takes a symbol or a proc
> that will be used to instantiate the aggregate object. It is optional
> and if not used then the existing behaviour of calling new with the
> mapped attributes will be used.
>
> 2. It deprecates the use of a block to provide a method of converting
> values assigned to the aggregate attribute. The use of a block didn’t
> feel particularly consistent with the rest of Rails where typically
> symbols or procs are passed as options (a good example being the :if
> and :unless options for validations). Of course passing a block will
> still function, so existing code won’t break, but it will raise a
> deprecation warning. This change leads on to…
>
> 3. It adds a new :converter option that also takes a symbol or proc
> that will be used when the aggregate attribute is assigned a value
> that is not an instance of the aggregate class. This replaces the old
> block syntax and makes it easier to do things like calling composed_of
> from within a with_options block. If both the :converter option and a
> block are passed to composed_of then the :converter option will take
> precedence.
>
> Feedback or, even better, +1''s appreciated...
>
> Thanks,
> Rob
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Hi Chris,

Thanks for the feedback, and for the original patches which I used for
inspiration (especially in the test cases!)

I''ve just updated the ticket on lighthouse with an example that better
illustrates the difference between the :constructor and :converter
procs.

While in some cases I think there will be similarities between
aggregate construction and conversion, I''d rather keep the two procs
with separate responsibilities for those cases where they are less
similar (if that makes sense).

Rob



On Aug 26, 3:31 pm, Chris Cruft <c...@hapgoods.com>
wrote:
> Rob,
> I like your patch: it solves problems I have had and the aesthetics of
> a proc feel right.  But the duplication between the constructor code
> and the converter code is not DRY.  Why not just lump them together
> into :constructor?  By providing the option to use a proc, you''ve
> already addressed the requirement to deal with differing constructor
> arguments:
>
> class Visitor
>   composed_of :ip_addr,
>               :class_name => ''IPAddr'',
>               :mapping => %w(ip to_i),
>               :constructor => Proc.new { |value| value.is_a?
> (Integer) ? IPAddr.new(value, Socket::AF_INET) :
> IPAddr.new(value.to_s) }
> end
>
> That should DRY things up a bit without sacrificing any functionality.
>
> PS - I''m the author of the patch (that got a "won''t
fix") you
> reference in your lighthouse ticket.
>
> On Aug 25, 1:59 pm, rob-twf <rob.ander...@googlemail.com> wrote:
>
> > Hi,
>
> > I''ve created a patch for composed_of
here:http://rails.lighthouseapp.com/projects/8994/tickets/892-composed_of-...
>
> > As summarised in the ticket:
>
> > 1. It adds a new :constructor option that takes a symbol or a proc
> > that will be used to instantiate the aggregate object. It is optional
> > and if not used then the existing behaviour of calling new with the
> > mapped attributes will be used.
>
> > 2. It deprecates the use of a block to provide a method of converting
> > values assigned to the aggregate attribute. The use of a block didn’t
> > feel particularly consistent with the rest of Rails where typically
> > symbols or procs are passed as options (a good example being the :if
> > and :unless options for validations). Of course passing a block will
> > still function, so existing code won’t break, but it will raise a
> > deprecation warning. This change leads on to…
>
> > 3. It adds a new :converter option that also takes a symbol or proc
> > that will be used when the aggregate attribute is assigned a value
> > that is not an instance of the aggregate class. This replaces the old
> > block syntax and makes it easier to do things like calling composed_of
> > from within a with_options block. If both the :converter option and a
> > block are passed to composed_of then the :converter option will take
> > precedence.
>
> > Feedback or, even better, +1''s appreciated...
>
> > Thanks,
> > Rob
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

FYI, Eloy Duran and I were also wrestling with composed_of and decided  
to implement a simpler alternative:

   http://github.com/Fingertips/attribute-decorator/tree/master

Manfred

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Slightly off-topic, but does anyone know _why_ the convention in  
options is to use :class_name => "Foo" instead of :class => Foo
?

I would like to make our attribute-decorator plugin use that instead  
of a string which has to be constantized.
Next to a light performance improvement, this would allow you to  
define a simple decorator wrapper class without any
magic. For the IPAddr example:

attribute_decorator :ip, :class => Class.new(IPAddr) do
   def self.parse(value)
     new(value.to_s, Socket::AF_UNSPEC)
   end

   def initialize(value, family = Socket::AF_INET)
     super
   end

   def to_a
     [to_i]
   end
end

Eloy

On Aug 27, 2008, at 11:51 AM, Manfred Stienstra wrote:
>
> FYI, Eloy Duran and I were also wrestling with composed_of and decided
> to implement a simpler alternative:
>
>   http://github.com/Fingertips/attribute-decorator/tree/master
>
> Manfred
>
> >

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Slightly off-topic, but does anyone know _why_ the convention in
options is to use :class_name => "Foo" instead of :class => Foo
?

I would like to make our attribute-decorator plugin use that instead
of a string which has to be constantized.
Next to a light performance improvement, this would allow you to
define a simple decorator wrapper class without any
magic. For the IPAddr example, if I understood the original example
correct that is :) :

attribute_decorator :ip, :class => Class.new(IPAddr) do
 def self.parse(value)
   new(value.to_s, Socket::AF_UNSPEC)
 end

 def initialize(value, family = Socket::AF_INET)
   super
 end

 def to_a
   [to_i]
 end
end

Eloy

On Aug 27, 11:51 am, Manfred Stienstra <manf...@gmail.com>
wrote:
> FYI, Eloy Duran and I were also wrestling with composed_of and decided  
> to implement a simpler alternative:
>
>    http://github.com/Fingertips/attribute-decorator/tree/master
>
> Manfred
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> Slightly off-topic, but does anyone know _why_ the convention in
> options is to use :class_name => "Foo" instead of :class =>
Foo ?
In this case I don''t think there''s a reason other than
consistency
with the associations, but in other parts of active record it''s
because sometimes those constants aren''t actually ready for use.  So
take:


class Bar
  belongs_to Foo
  def self.some_method
  end
end

class Foo
  has_many :whatevers, :class=>Bar
end

In the has_many call,  Foo.respond_to? :some_method returns false
because that method hasn''t been defined yet.
> I would like to make our attribute-decorator plugin use that instead
> of a string which has to be constantized.
> Next to a light performance improvement, this would allow you to
> define a simple decorator wrapper class without any
> magic. For the IPAddr example, if I understood the original example
> correct that is :) :
Your plugin is pretty interesting stuff, it seems it could be a nice
replacement for composed_of at some stage in the near future?  Have
you put any thought into turning it into a patch?

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> In this case I don''t think there''s a reason other than
consistency
> with the associations, but in other parts of active record it''s
> because sometimes those constants aren''t actually ready for use.
Ok that makes sense and is basically what I expected.
I''ll add the :class option then.
> Your plugin is pretty interesting stuff, it seems it could be a nice
> replacement for composed_of at some stage in the near future?  Have
> you put any thought into turning it into a patch?
Thanks.

At the moment we''re very busy with an application that uses this
plugin.
Once we think it has matured enough we''d love to turn this into a
patch,
this is why we wrote it as a plugin with lots of docs etc to begin  
with :)

This would be in the not too distant future and if enough people are  
interested in said patch.

Eloy

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I looked at the proposed attribute-decorator plugin a bit and found it
pretty simple.  But (assuming I understood it) the simplicity comes
with a noticeable penalty for the use case where you want to use a
standard class for the decorator that does not include a parse factory
method: another wrapper or subclass that provides a parse method.

For example, the canonical example of Money (and IPAddr, etc.) would
require yet another wrapper for the sole purpose of providing a parse
method.

Or did I miss something?

Assuming I didn''t, it seems to be a step backwards to not be able to
use the "new" constructor, or better yet, a configurable constructor
that would accomodate "parse", "new",
"from_widget" or whatever.

On another note, the semantics of "parse" imply (to me at least)
extracting tokens/objects from a string, whereas in attribute-
decorator it seems the "parse" method is intended to handle conversion
to the aggregate class even when the factory parameters are not
strings.  Perhaps "coerce" wouldn''t carry as much baggage.

-Chris


On Aug 27, 10:01 am, Eloy Duran <eloy.de.en...@gmail.com>
wrote:
> > In this case I don''t think there''s a reason other
than consistency
> > with the associations, but in other parts of active record
it''s
> > because sometimes those constants aren''t actually ready for
use.
>
> Ok that makes sense and is basically what I expected.
> I''ll add the :class option then.
>
> > Your plugin is pretty interesting stuff, it seems it could be a nice
> > replacement for composed_of at some stage in the near future?  Have
> > you put any thought into turning it into a patch?
>
> Thanks.
>
> At the moment we''re very busy with an application that uses this
plugin.
> Once we think it has matured enough we''d love to turn this into a
patch,
> this is why we wrote it as a plugin with lots of docs etc to begin  
> with :)
>
> This would be in the not too distant future and if enough people are  
> interested in said patch.
>
> Eloy
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Aug 28, 2:03 pm, Chris Cruft <c...@hapgoods.com>
wrote:
> I looked at the proposed attribute-decorator plugin a bit and found it
> pretty simple.  But (assuming I understood it) the simplicity comes
> with a noticeable penalty for the use case where you want to use a
> standard class for the decorator that does not include a parse factory
> method: another wrapper or subclass that provides a parse method.
The simplicity of the implementation comes from the fact that we
assume a number on methods on the value objects.
> For example, the canonical example of Money (and IPAddr, etc.) would
> require yet another wrapper for the sole purpose of providing a parse
> method.
We''ve found that directly sending a value from the form to a value
object is almost never enough.

If you ask a user to fill out an amount, it''s a bit strange to require
them to fill it out in cents. A much nicer format would be "$10.50",
"10.50", or maybe even "10 dollar". A Money object only
takes an
integer amount of cents in the initializer so you will always have to
do some amount of parsing.

In the example of an IP address you might want to automatically
discover if we''re dealing with IPv4 or IPv6 addresses, which is a
required second parameter in the constructor.
> Assuming I didn''t, it seems to be a step backwards to not be able
to
> use the "new" constructor, or better yet, a configurable
constructor
> that would accomodate "parse", "new",
"from_widget" or whatever.
I believe that configuration options are what made composed_of
unusable in the first place.
> On another note, the semantics of "parse" imply (to me at least)
> extracting tokens/objects from a string, whereas in attribute-
> decorator it seems the "parse" method is intended to handle
conversion
> to the aggregate class even when the factory parameters are not
> strings.  Perhaps "coerce" wouldn''t carry as much
baggage.
Following my argument above about Money, I think parse is perfectly in
order. All form values are passed as strings and have to be
internalized somehow.

A simple alias from coerce to parse would be enough to describe the
intentions of your value object.

Manfred
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Manfred,
Thanks for your clear explanation.  I now see what you focus on with
attribute-decorator: dealing with HTML forms.  I must admit that is a
potentially big part of the equation and the persisted values in the
DB might only be a small piece.  I guess it depends on your
application: where the source data comes from and how it is stored.

I''ve got to mull this one over now...

-Chris

On Aug 28, 9:23 am, Manfred Stienstra <manf...@gmail.com>
wrote:
> On Aug 28, 2:03 pm, Chris Cruft <c...@hapgoods.com> wrote:
>
> > I looked at the proposed attribute-decorator plugin a bit and found it
> > pretty simple.  But (assuming I understood it) the simplicity comes
> > with a noticeable penalty for the use case where you want to use a
> > standard class for the decorator that does not include a parse factory
> > method: another wrapper or subclass that provides a parse method.
>
> The simplicity of the implementation comes from the fact that we
> assume a number on methods on the value objects.
>
> > For example, the canonical example of Money (and IPAddr, etc.) would
> > require yet another wrapper for the sole purpose of providing a parse
> > method.
>
> We''ve found that directly sending a value from the form to a value
> object is almost never enough.
>
> If you ask a user to fill out an amount, it''s a bit strange to
require
> them to fill it out in cents. A much nicer format would be
"$10.50",
> "10.50", or maybe even "10 dollar". A Money object only
takes an
> integer amount of cents in the initializer so you will always have to
> do some amount of parsing.
>
> In the example of an IP address you might want to automatically
> discover if we''re dealing with IPv4 or IPv6 addresses, which is a
> required second parameter in the constructor.
>
> > Assuming I didn''t, it seems to be a step backwards to not be
able to
> > use the "new" constructor, or better yet, a configurable
constructor
> > that would accomodate "parse", "new",
"from_widget" or whatever.
>
> I believe that configuration options are what made composed_of
> unusable in the first place.
>
> > On another note, the semantics of "parse" imply (to me at
least)
> > extracting tokens/objects from a string, whereas in attribute-
> > decorator it seems the "parse" method is intended to handle
conversion
> > to the aggregate class even when the factory parameters are not
> > strings.  Perhaps "coerce" wouldn''t carry as much
baggage.
>
> Following my argument above about Money, I think parse is perfectly in
> order. All form values are passed as strings and have to be
> internalized somehow.
>
> A simple alias from coerce to parse would be enough to describe the
> intentions of your value object.
>
> Manfred
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I do like some of the ideas in the attribute-decorator code. Here''s
what I don''t like (these are quite similar to those concerns posted by
Chris and I appreciate this has been developed for specific
requirements rather than for general use):

1. The anonymous class approach as shown in this thread for the IPAddr
example is nice in that it does mean I don''t need to create a special
class just for aggregation, however I don''t like the idea of forcing
methods (parse, to_a and optionally valid?) into a class to make this
work. Also what happens if a class already has these methods, they
don''t provide the behaviour needed for attribute decoration but are
needed elsewhere in the code?

2. The use of a special class for aggregation, as in the CompositeDate
example from the source code, is good for DRYness as I can write my
parse and to_a methods in one place and all models that use the same
aggregations can use the same code (as an aside you could of course
achieve something similar by putting specialised versions of
composed_of in a module). However it doesn''t feel right that my
aggregated attribute is now an instance of this class: again using the
CompositeDate example, I''d like the date_of_birth attribute to be a
Date object, not a CompositeDate.

Perhaps a better solution would be to employ some mapping code that
simply handles conversion/parsing, something which is a little closer
to the composition generator posted by another Chris near the start of
this thread. Decorated attributes would then be of the expected class
and that class would not need to have any new methods grafted on.

Having thought about it, could we not use something like the
association extension syntax to do this? For example:
http://www.pastie.org/263192

Also following the association extension syntax, we could place
commonly used aggregations into a module and pass an :extend option to
attribute_decorator like so:

attribute_decorator :date_of_birth, :decorates =>
[:day, :month, :year], :extend => CompositeDateExtension


I''ve used attribute_decorator as the method name here, although it
could just as easily be composed_of but that would mean breakage for
the two or three people currently using composed_of :)

I can understand the choice of parse over coerce as previously
discussed, however I would like to find a better, more descriptive
name for to_a: I know it *is* turning attributes into an array, but
that''s really an implementation detail, the name doesn''t
really tell
us why it is doing that. I was thinking along the lines of mapping or
map but they''re not really working for me either.

Obviously more thought is needed but what do you think?
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I realised after posting that I''d forgotten to add a constructor to my
example...and that led me on to two things: replacing positional
arguments with a hash (more Railsy) and changing the to_a method to an
attributes reader that returns a hash. Here''s an updated example:
http://www.pastie.org/263243
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Actually the point is to not focus on a specific situation,
but rather let you easily define decorators for any purpose.
I will respond more in depth on this in my reply to Rob''s emails.

Eloy

On 29 aug 2008, at 13:42, Chris Cruft wrote:
>
> Manfred,
> Thanks for your clear explanation.  I now see what you focus on with
> attribute-decorator: dealing with HTML forms.  I must admit that is a
> potentially big part of the equation and the persisted values in the
> DB might only be a small piece.  I guess it depends on your
> application: where the source data comes from and how it is stored.
>
> I''ve got to mull this one over now...
>
> -Chris
>
> On Aug 28, 9:23 am, Manfred Stienstra <manf...@gmail.com> wrote:
>> On Aug 28, 2:03 pm, Chris Cruft <c...@hapgoods.com> wrote:
>>
>>> I looked at the proposed attribute-decorator plugin a bit and  
>>> found it
>>> pretty simple.  But (assuming I understood it) the simplicity comes
>>> with a noticeable penalty for the use case where you want to use a
>>> standard class for the decorator that does not include a parse  
>>> factory
>>> method: another wrapper or subclass that provides a parse method.
>>
>> The simplicity of the implementation comes from the fact that we
>> assume a number on methods on the value objects.
>>
>>> For example, the canonical example of Money (and IPAddr, etc.)
would
>>> require yet another wrapper for the sole purpose of providing a  
>>> parse
>>> method.
>>
>> We''ve found that directly sending a value from the form to a
value
>> object is almost never enough.
>>
>> If you ask a user to fill out an amount, it''s a bit strange to
>> require
>> them to fill it out in cents. A much nicer format would be
"$10.50",
>> "10.50", or maybe even "10 dollar". A Money object
only takes an
>> integer amount of cents in the initializer so you will always have to
>> do some amount of parsing.
>>
>> In the example of an IP address you might want to automatically
>> discover if we''re dealing with IPv4 or IPv6 addresses, which
is a
>> required second parameter in the constructor.
>>
>>> Assuming I didn''t, it seems to be a step backwards to not
be able to
>>> use the "new" constructor, or better yet, a configurable
constructor
>>> that would accomodate "parse", "new",
"from_widget" or whatever.
>>
>> I believe that configuration options are what made composed_of
>> unusable in the first place.
>>
>>> On another note, the semantics of "parse" imply (to me at
least)
>>> extracting tokens/objects from a string, whereas in attribute-
>>> decorator it seems the "parse" method is intended to
handle
>>> conversion
>>> to the aggregate class even when the factory parameters are not
>>> strings.  Perhaps "coerce" wouldn''t carry as
much baggage.
>>
>> Following my argument above about Money, I think parse is perfectly  
>> in
>> order. All form values are passed as strings and have to be
>> internalized somehow.
>>
>> A simple alias from coerce to parse would be enough to describe the
>> intentions of your value object.
>>
>> Manfred
> >

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> I do like some of the ideas in the attribute-decorator code.
Here''s
> what I don''t like (these are quite similar to those concerns
posted by
> Chris and I appreciate this has been developed for specific
> requirements rather than for general use):
While in Berlin for RailsConf this week, we''ll probably have a stab at
creating a patch out of this.
I think that discussing this at length will in the end bring us nowhere,
so it might be a better idea if you would create your own implementation
so we have something real to compare and discuss. Which is especially  
important for the core team.

Having said that I will try to respond to each of your concerns as  
good as possible about what our ideas are :)
> 1. The anonymous class approach as shown in this thread for the IPAddr
> example is nice in that it does mean I don''t need to create a
special
> class just for aggregation, however I don''t like the idea of
forcing
> methods (parse, to_a and optionally valid?) into a class to make this
> work.
Well actually we are in fact defining a new wrapper class with  
Class.new,
which would probably be done under the hood anyway if we would take  
for instance a block for attribute_decorator.
My main concern is that the decorator layer shouldn''t try to be to  
smart and thus add bloat
code for things than can just as easily be handled by the existing  
standard Ruby tools.
Like Class.new with a block and using ducktyping conventions like to_a  
and to_s (which I failed to include in the IPAddr example...).

Also I would probably only use this Class.new way in the most simple  
cases,
because in more typical cases I''d like to have the logic nicely tugged
away in it''s
own class allowing better testability.
> Also what happens if a class already has these methods, they
> don''t provide the behaviour needed for attribute decoration but
are
> needed elsewhere in the code?

Well the example I gave where I subclassed IPAddr was just one way of  
doing it.
The beauty in not assuming too much, is that you as the developer are  
free
to solve it in any way as you see fit.

Back to your example. If the class that''s being wrapped already has  
these methods
defined, you can either use super, as is actually shown in the IPAddr  
example,
or write a proxy class which delegates to the IPAddr class in the  
correct manner.

Both are IMO very sane alternatives, as is with any other Ruby class.
In other words, there shouldn''t be anything new to people in doing it  
this way,
because it''s all plain Ruby that they could/should already know.

Something which I find absolutely not to be the case with convoluted
methods like composed_of, which try to do everything but then fail to  
do the
one thing they should do in a concise, but more importantly, readable  
manner.
> 2. The use of a special class for aggregation, as in the CompositeDate
> example from the source code, is good for DRYness as I can write my
> parse and to_a methods in one place and all models that use the same
> aggregations can use the same code (as an aside you could of course
> achieve something similar by putting specialised versions of
> composed_of in a module).
More importantly, it separates the code more and makes it better  
testable like
any other Ruby class. Which would be harder when you need a model
context for it to work, because the aggregation logic is part of the  
model
instead of it''s own independent class.
> However it doesn''t feel right that my
> aggregated attribute is now an instance of this class: again using the
> CompositeDate example, I''d like the date_of_birth attribute to be
a
> Date object, not a CompositeDate.
Well, like shown in the IPAddr example, you could make CompositeDate a  
subclass of Date
and add your specific logic there, it should then still walk and quack  
like a Date instance
and even be an instance_of?(Date). Solve it as you see fit for each  
specific situation.
> Perhaps a better solution would be to employ some mapping code that
> simply handles conversion/parsing, something which is a little closer
> to the composition generator posted by another Chris near the start of
> this thread. Decorated attributes would then be of the expected class
> and that class would not need to have any new methods grafted on.
Like said before, this is what attribute_decorator already allows you  
to do,
we expect you to have implemented the proper methods for conversion/ 
parsing to work
and the returned instance can be of the expected class, or rather a  
subclass, if you wish so.
> Having thought about it, could we not use something like the
> association extension syntax to do this? For example:
http://www.pastie.org/263192
The only benefit I see in this approach, is that creating an anonymous  
class/module is done
behind the hood instead of doing it yourself. Basically saving you  
from having to write: ":class => Class.new do … end"
This shoves the logic for how to create this class into the  
aggregation layer, where it doesn''t belong.
You will only limit yourself to all available options by doing it this  
way.
> Also following the association extension syntax, we could place
> commonly used aggregations into a module and pass an :extend option to
> attribute_decorator like so:
>
> attribute_decorator :date_of_birth, :decorates =>
> [:day, :month, :year], :extend => CompositeDateExtension
Or you could simply define a module which does this when included:

module DateOfBirthDecorator
   def self.included(klass)
     klass.attribute_decorator :date_of_birth, :decorates =>  
[:day, :month, :year], :class => CompositeDate
   end
end

Then it would only be a matter of doing:

class Member < ActiveRecord::Base
   include DateOfBirthDecorator
end

class Artist < ActiveRecord::Base
   include DateOfBirthDecorator
end

Etc.

I don''t see a real need to abstract this any further...

It all boils down, for us at least, to the fact that Ruby itself  
already provides
us with everything needed in these cases. So the mapping layer
should be as thin and clear as possible, instead of re-inventing the  
wheel.
Thus making it easier to debug AND be more flexible at the same time.
> I''ve used attribute_decorator as the method name here, although it
> could just as easily be composed_of but that would mean breakage for
> the two or three people currently using composed_of :)
>
> I can understand the choice of parse over coerce as previously
> discussed, however I would like to find a better, more descriptive
> name for to_a: I know it *is* turning attributes into an array, but
> that''s really an implementation detail, the name doesn''t
really tell
> us why it is doing that. I was thinking along the lines of mapping or
> map but they''re not really working for me either.
> Obviously more thought is needed but what do you think?
I''m not 100% sure it should need to tell us why, because it simply  
implements a protocol.

The attributes hash, as you mention in your next email, could be an  
alternative
to this. But then again, you need to decide if we should also send a  
hash to the initializer,
which will only lengthen your code.

With an array, the protocol is to simply take and return the  
attributes in the same order as
specified in the :decorates option, it is a pretty simple convention  
IMO.

Cheers,
Eloy
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I have submitted a patch based on our AttributeDecorator plugin:
http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/950-attributedecorator-a-new-take-on-aggregation

Eloy

On 31 aug 2008, at 18:41, Eloy Duran wrote:
>> I do like some of the ideas in the attribute-decorator code.
Here''s
>> what I don''t like (these are quite similar to those concerns
posted
>> by
>> Chris and I appreciate this has been developed for specific
>> requirements rather than for general use):
>
> While in Berlin for RailsConf this week, we''ll probably have a
stab
> at creating a patch out of this.
> I think that discussing this at length will in the end bring us  
> nowhere,
> so it might be a better idea if you would create your own  
> implementation
> so we have something real to compare and discuss. Which is  
> especially important for the core team.
>
> Having said that I will try to respond to each of your concerns as  
> good as possible about what our ideas are :)
>
>> 1. The anonymous class approach as shown in this thread for the  
>> IPAddr
>> example is nice in that it does mean I don''t need to create a
special
>> class just for aggregation, however I don''t like the idea of
forcing
>> methods (parse, to_a and optionally valid?) into a class to make this
>> work.
>
> Well actually we are in fact defining a new wrapper class with  
> Class.new,
> which would probably be done under the hood anyway if we would take  
> for instance a block for attribute_decorator.
> My main concern is that the decorator layer shouldn''t try to be to
> smart and thus add bloat
> code for things than can just as easily be handled by the existing  
> standard Ruby tools.
> Like Class.new with a block and using ducktyping conventions like  
> to_a and to_s (which I failed to include in the IPAddr example...).
>
> Also I would probably only use this Class.new way in the most simple  
> cases,
> because in more typical cases I''d like to have the logic nicely  
> tugged away in it''s
> own class allowing better testability.
>
>> Also what happens if a class already has these methods, they
>> don''t provide the behaviour needed for attribute decoration
but are
>> needed elsewhere in the code?
>
>
> Well the example I gave where I subclassed IPAddr was just one way  
> of doing it.
> The beauty in not assuming too much, is that you as the developer  
> are free
> to solve it in any way as you see fit.
>
> Back to your example. If the class that''s being wrapped already
has
> these methods
> defined, you can either use super, as is actually shown in the  
> IPAddr example,
> or write a proxy class which delegates to the IPAddr class in the  
> correct manner.
>
> Both are IMO very sane alternatives, as is with any other Ruby class.
> In other words, there shouldn''t be anything new to people in doing
> it this way,
> because it''s all plain Ruby that they could/should already know.
>
> Something which I find absolutely not to be the case with convoluted
> methods like composed_of, which try to do everything but then fail  
> to do the
> one thing they should do in a concise, but more importantly,  
> readable manner.
>
>> 2. The use of a special class for aggregation, as in the  
>> CompositeDate
>> example from the source code, is good for DRYness as I can write my
>> parse and to_a methods in one place and all models that use the same
>> aggregations can use the same code (as an aside you could of course
>> achieve something similar by putting specialised versions of
>> composed_of in a module).
>
> More importantly, it separates the code more and makes it better  
> testable like
> any other Ruby class. Which would be harder when you need a model
> context for it to work, because the aggregation logic is part of the  
> model
> instead of it''s own independent class.
>
>> However it doesn''t feel right that my
>> aggregated attribute is now an instance of this class: again using  
>> the
>> CompositeDate example, I''d like the date_of_birth attribute to
be a
>> Date object, not a CompositeDate.
>
> Well, like shown in the IPAddr example, you could make CompositeDate  
> a subclass of Date
> and add your specific logic there, it should then still walk and  
> quack like a Date instance
> and even be an instance_of?(Date). Solve it as you see fit for each  
> specific situation.
>
>> Perhaps a better solution would be to employ some mapping code that
>> simply handles conversion/parsing, something which is a little closer
>> to the composition generator posted by another Chris near the start  
>> of
>> this thread. Decorated attributes would then be of the expected class
>> and that class would not need to have any new methods grafted on.
>
> Like said before, this is what attribute_decorator already allows  
> you to do,
> we expect you to have implemented the proper methods for conversion/ 
> parsing to work
> and the returned instance can be of the expected class, or rather a  
> subclass, if you wish so.
>
>> Having thought about it, could we not use something like the
>> association extension syntax to do this? For example:
http://www.pastie.org/263192
>
> The only benefit I see in this approach, is that creating an  
> anonymous class/module is done
> behind the hood instead of doing it yourself. Basically saving you  
> from having to write: ":class => Class.new doend"
> This shoves the logic for how to create this class into the  
> aggregation layer, where it doesn''t belong.
> You will only limit yourself to all available options by doing it  
> this way.
>
>> Also following the association extension syntax, we could place
>> commonly used aggregations into a module and pass an :extend option  
>> to
>> attribute_decorator like so:
>>
>> attribute_decorator :date_of_birth, :decorates =>
>> [:day, :month, :year], :extend => CompositeDateExtension
>
> Or you could simply define a module which does this when included:
>
> module DateOfBirthDecorator
>  def self.included(klass)
>    klass.attribute_decorator :date_of_birth, :decorates =>  
> [:day, :month, :year], :class => CompositeDate
>  end
> end
>
> Then it would only be a matter of doing:
>
> class Member < ActiveRecord::Base
>  include DateOfBirthDecorator
> end
>
> class Artist < ActiveRecord::Base
>  include DateOfBirthDecorator
> end
>
> Etc.
>
> I don''t see a real need to abstract this any further...
>
> It all boils down, for us at least, to the fact that Ruby itself  
> already provides
> us with everything needed in these cases. So the mapping layer
> should be as thin and clear as possible, instead of re-inventing the  
> wheel.
> Thus making it easier to debug AND be more flexible at the same time.
>
>> I''ve used attribute_decorator as the method name here,
although it
>> could just as easily be composed_of but that would mean breakage for
>> the two or three people currently using composed_of :)
>>
>> I can understand the choice of parse over coerce as previously
>> discussed, however I would like to find a better, more descriptive
>> name for to_a: I know it *is* turning attributes into an array, but
>> that''s really an implementation detail, the name
doesn''t really tell
>> us why it is doing that. I was thinking along the lines of mapping or
>> map but they''re not really working for me either.
>
>> Obviously more thought is needed but what do you think?
>
> I''m not 100% sure it should need to tell us why, because it simply
> implements a protocol.
>
> The attributes hash, as you mention in your next email, could be an  
> alternative
> to this. But then again, you need to decide if we should also send a  
> hash to the initializer,
> which will only lengthen your code.
>
> With an array, the protocol is to simply take and return the  
> attributes in the same order as
> specified in the :decorates option, it is a pretty simple convention  
> IMO.
>
> Cheers,
> Eloy

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---