Hi Stevan,
Thanks for the review. At the current point, my best response would be only to
include your quoted text in the "COMPATIBILITY" section of the pod in 0.02,
because I have no idea how to work around these problems - except for the
{...@_}
note, which I can fix, and for ArrayRef/HashRef note, which I think I'll be
able to deal with.
This means that I welcome patches and advices that would not only help me fix
the rest of incompatibilities for today, but also would be in sync with the
main ideology of Moose.
Thanks again,
/dk
On Wed, Jan 13, 2010 at 07:23:20PM -0500, Stevan Little wrote:
> Dmitry,
>
> Here is my observations on the code you posted for review.
>
> I would have to agree with Dieter on this, overloading 'isa' to no
> longer mean "the type constraint to use for validation" is not good.
> In my experience, overloaded terminology like this often leads to
> incorrect assumptions.
>
> And since you do not actually replace 'isa' with the correct the type
> an ArrayRef or HashRef, which is after all what you are storing, you
> lose the validation all together. This means that if people use the
> 'default' or 'builder' option and their code accidently stores
> something incorrect, they will not get the error from the validation
> and very likely the error will manifest later on in the code when it
> is harder to trace.
>
> Your code also makes the assumption that $name will always be the name
> of the accessor. So this will not work as you expect:
>
> has_list foo => (
> accessor => '_foo',
> isa => 'Array',
> );
>
> It will attempt to wrap a non-existant 'foo' method. Your code will
> also fail for the following:
>
> has_list foo => (
> reader => 'get_foo',
> writer => 'set_foo',
> isa => 'Array',
> );
>
> Here you need to wrap both the get_foo and set_foo methods, each in a
> different way.
>
> You are also not checking that Hash accessors will correctly construct
> a hash, you should at least check to be sure that @_ contains an even
> number of arguments. Code like this will work, but will give a cryptic
> warning (Odd number of elements in anonymous hash at lib//MooseX/
> Lists.pm line 33.).
>
> Your code will also fail with the following definition:
>
> has_list foo => (
> is => 'rw',
> isa => 'Array',
> clearer => 'clear_foo'
> );
>
> When you call clear_foo it will remove the value stored in 'foo', and
> then when you call $object->foo the next time you will get "Can't use
> an undefined value as an ARRAY reference at lib//MooseX/Lists.pm line
> 22." as an error.
>
> Thats about all I see for now, if you want to repost 0.02 we can
> review again.
>
> - Stevan
--
Sincerely,
Dmitry Karasik
