John Peacock wrote:
> Michael G Schwern wrote:
>> Go ahead and use isa() to check the class of the version object. Can't hurt.
>
> ...said the man who wasn't working on the patch. ;-)
...said the man who doesn't really want it in the first place. :P But if
we're going to do it we're going to do it right.
Don't worry, I'll stop torturing you soon.
> OK, here's my compromise: we still use ref for the first pass through (so it
> will handle the base version objects)
Hold up. Why won't isa() work on base version objects? What's a "base"
version object? Did you instead mean so it will work on non-object references
(ie. hash refs, etc...)? Or did you mean normal string/number versions?
> and we try real hard to use isa() safely
> if that fails. Here's that block of the diff:
>
> @@ -120,8 +121,8 @@
> }
>
> my @sigs = ref $sig ? @$sig : $sig;
> - my $given = lc ref $val;
> - unless( grep $given eq $_, @sigs ) {
> + my $given = ref $val;
> + unless( grep { $given eq $_ || ($_ && eval{$val->isa($_)}) } @sigs )
> {
> my $takes = join " or ", map { $_ ne '' ? "$_ reference"
> : "string/number"
> } @sigs;
>
> I'm going to write a test that exercises this code, but I already confirmed
> that
> a subclass of version will fail the first || term and pass the second. Now
> that
> I look at it again, I'd like to rewrite that grep block to be
>
> { $_ && ($given eq $_ || eval{$val->isa($_)) }
>
> which is (to my mind) a little clearer about the intent of the tests. Of
> course
> I personally would also edit the next line to eliminate the useless "ne ''"
> (which isn't clearer to my eyes).
If you eliminate that how will it display the right message when it accepts a
non-reference?
The code looks ok to me. Stick it on RT as a patch so I won't forget it.
