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
On Jan 13, 2010, at 6:50 PM, Dmitry Karasik wrote:
'isa' is potentially confusing, because you're using it differently
from the
rest of Moose. You could
* rename 'isa' to something else (I have no good ideas)
* have has_array and has_hash instead
* use isa => 'ArrayRef' and 'HashRef', as usual, and just add the
around()
based on whichever of them is present
Also, 02-syntax.t doesn't actually test the modifiers you're adding
-- if you
add 'main->new->a2' at the end, you'll see it dies because you have
a reference to
'array_ref' (which doesn't exist).
The fact that you're saying this here after being rude on irc channel,
makes me think that you're just trolling at this point.
--
Sincerely,
Dmitry Karasik
