Re: [code-review] Please review String::FlexMatch

I've finally got to look at the module and the tests. Now I see what you're 
doing with deep tests. This is something I wanted to do before and it became 
Test::Deep. I wanted is_deeply to examine the class of blessed objects. I 
suggested that we could make it check class if the expected argument was 
blessed, otherwise it would ignore it. Michael said no way, is_deeply 
shouldn't be changing it's behaviour depending on the data you put in.

I'd say you should be using Test::Deep instead of putting overloaded objects 
into is_deeply (at the end I've rewritten one of your tests using it). For 
me, the fact that it used to work previously is a bug. I haven't heard 
Michael's opinion about it yet though.

Below are some more arguments for why is_deeply shouldn't do it.

On Thursday 04 September 2003 15:10, Marcel Grünauer wrote:
> But I think with overloading it's ok, because in using overload, the 
> object
> *asks* for it. In fact, if an object provides an overloaded 
> stringification,
> it seems to be an error to try and circumvent it.

But the object isn't asking for string-wise equality, it's asking for deep 
equality and the only thing that's deeply equal to "string" is "string", not 
an object that is pretending to be "string". Deep is the key word here. The 
docs don't say anything about string equality, they say that is_deeply will 
walk the structures and that should include the structures of an overloaded 
object.

What if you had overloaded eq on $ref and were accidentally always returning 
true? is_deeply($ref, "any string") would be a pass.

I thought of another example of why it's wrong. Take $o1, $o2 and "string". 
Say $o1 and $o2 both stringify to "string" but they are completely different 
objects so. $o1 is deeply equal to "string", "string" is deeply equal to $o2 
but $o1 and $o2 are not deeply equal to each other. This is not good.

> 'eq' still tests for string equality, not if an object really is the 
> same.
> The latter may be the intent of _deep_check() and is_deeply() and 
> friends,
> but using 'eq' I expect to compare stringifications.
> 
> As for which method to check, using 'eq' to compare strings seems to ask
> for overload::Method($o, '""').

Nothing in the docs says that is_deeply will use eq. I would guess that the 
reason it does is because eq can correctly compare numbers and strings and 
will spot that "123" and "123a" are not the same. It is purely an 
implementation detail and so could be subject to change.

Like I said, I've asked Michael Schwern for his take.

> The whole point of the module was to provide a way of transparently 
> offering
> different methods of comparing a string with a thing (via string, regex,
> coderef) and anyone using 'eq' should never know the difference. If 
> standard
> test methods won't work, I'll have to write special versions of 
> _deep_check()
> et al, which would rather defeat the purpose of the module - if I know 
> that
> I'm dealing with a special case, I don't need an object for that, I can 
> just
> test what I'm comparing against.

The special version has been written already. Test::Deep handles everything 
you want to do and much more. So instead of you writing a special version of 
_deep_check(), I've written it already and it's designed to be special 
caseable.

The docs may be a bit difficult to understand, opinions appreciated. Anyway, 
here's your eq_hash test

my $hash1 = {
  errors => {
    attr1 => 'A pure string',
    attr2 => '/home/marcel/lib/Foo/Bar.pm',
    attr3 => 58,
    attr4 => 'No such class',
  },
};

my $exp = {
  errors => {
    attr1 => "A pure string",
    attr2 => re(qr#.*/lib/Foo/Bar.pm#),
    attr3 => re("/'\d+'/"),
    attr4 => "No such class"
  }
}
$hash1->{attr5} = 'This should not be here';
cmp_deeply($hash1, $exp, "Hash with extra key");

It's been around for months now, it's stable and it's easy to extend. Please 
have a look and see if it does what you need,

F