Dmitry,
On Jan 13, 2010, at 7:45 PM, Dmitry Karasik wrote:
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.
They are all possible to work around, but in some cases it will just
open up more issues. The Meta::Attribute objects have accessors that
will determine for you the reader and writer methods. You will also
need to account for the (is => 'bare') option, which is how you tell
Moose "I don't want an accessor".
This means that I welcome patches and advices
I neither have the time or the inclination to provide a patch, I
prefer to work with references only, so advice is all I can provide.
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.
Well, not overloading "isa" would bring you closer to the main
ideology of Moose. Changing these to meta-attribute traits would also
help as we tend to discourage adding more "keywords". And lastly
supporting the items I mentioned in the previous email would help as
well. Most MooseX modules try very hard to work with other existing
Moose features as well as other MooseX modules, you module simply
makes too many assumptions to do so.
- Stevan
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
