I'm not sure is the 'why it's wrong' is as compelling as a 'how could it be
better argument'. (TIMTOWDI and NIH are alive!) In this particular example
it looks like a custom type style usage (maybe entirely unnecessary but
still) for custom types there are already several great (Moo(se)y) ways to
handle them. for example http://search.cpan.org/~tobyink/Type-Tiny/.
Jed
On Thu, Aug 1, 2013 at 8:42 AM, Chris Prather <perig...@prather.org> wrote:
> Well they're relying on undefined behavior. Roles are defined to only
> compose *methods* not functions and relying on what they're doing a
> something that will "work" isn't guranteed to always work. Given a testcase
> and a patch that fixes it (but doesn't break anything else) I would argue
> Moose should adopt the more correct behavior of forbidding that.
>
> I don't think that patch would be trivial to write, so I'm not sure you
> can rely upon that as an argument with whomever is adding this to your
> codebase at work.
>
> If these roles are *only* doing this undefined "Exporter" like behavior,
> they're paying a performance penalty because the Moose code has to set up
> all the meta layers for Roles the exporting that Sub::Exporter (a dep from
> Moose) or Exporter itself (a core module) don't have to setup.
>
> Beyond that, it's bad form and practice. It is the moral equivalent of
> using a hammer to put screws in the wall.
>
> -Chris
>
>
> On Thu, Aug 1, 2013 at 11:21 AM, Bill Moseley <mose...@hank.org> wrote:
>
>> I just noticed this code again a work, and then found this email that I
>> don't see every received a response.
>>
>> Can anyone provide their insight?
>>
>>
>> On Sun, Sep 18, 2011 at 11:34 PM, Bill Moseley <mose...@hank.org> wrote:
>>
>>> I'm on vacation and have finally found wireless to check out the svn
>>> logs back at work. I found a bunch of new Moose Roles created that are
>>> being used like one might use Exporter. Here's an example:
>>>
>>> package App::Role::Date;
>>> use strict;
>>> use warnings;
>>> use Moose::Role;
>>> use DateTime;
>>> use Carp;
>>>
>>>
>>> sub date_is_in_future {
>>> my ( $test_date ) = @_;
>>>
>>> croak 'first arg to date_is_in_future must be DateTime'
>>> unless ref( $test_date ) eq 'DateTime';
>>>
>>> my $now = DateTime->now();
>>>
>>> if ( $test_date >= $now ) {
>>> return 1;
>>> }
>>>
>>> return 0;
>>> }
>>>
>>> Then in a number of classes (most not Moose-based to start with) that
>>> have attributes that hold a DateTime there's code added like this:
>>>
>>> with 'App::Role::Date';
>>>
>>> ...
>>> my $dt = $self->start_time;
>>> if ( date_is_in_future( $dt ) ) {
>>> ....
>>> }
>>>
>>> Perhaps I'm being pedantic, but that makes my skin crawl. For one
>>> thing it's using a Role like Exporter. And "date_is_in_future" seems like
>>> it should be a method on the DateTime object (although seems "$dt >
>>> DateTime->now" is just as easy to write.) And, of course, the above will
>>> fail on DateTime subclasses. Not to mention returning 0 or 1 gives me the
>>> willies.
>>>
>>> Are there other more Moose-specific reasons that this code
>>> is problematic that I can point out?
>>>
>>>
>>>
>>> --
>>> Bill Moseley
>>> mose...@hank.org
>>>
>>
>>
>>
>> --
>> Bill Moseley
>> mose...@hank.org
>>
>
>
