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

On Sun, Sep 07, 2003 at 11:15:42PM +0100, Fergal Daly wrote:
> Long mail, I've moved the summary to the front.
> Currently is_deeply's definition of deep equality is
> - based purely on data, not behaviour

Nope. Consider tying.

> - intuitive: like the definitions of other kinds of equality it is symmetric, 
> reflexive and transitive

For values of intuitive that mean intuitive to you but not to me...

> - as deep as possible, something shouldn't claim to be deep but change it's 
> mind sometimes.

Again, I think you're using a different concept of deep from me.

> - non-destructive

I don't know what you mean by destructive, but again I expect that tying
gives the lie to this.

> - strict

??

> - leaves no room for confusion or doubt about what it does

Yes it does, or we wouldn't be having this conversation.

> The definition you want
> - includes behaviour, 

As does the current version.

> sometimes and for some reason "" is more important than 
> 0+ overloading

I've never suggested this. I've only suggested that when overloading is
in place that is and is_deeply should do the same thing, ideally by
using the same code.

> - is neither symmetric nor transitive and so is unlike other types of equality

See above.

> - is not always as deep as it could be

That is correct.

> - can possibly alter the data

As can the current behaviour:

	package Tie::Inc;

	sub TIESCALAR { bless [ $_[1] ] => $_[0]; }
	sub STORE     { $_[0]->[0] = $_[1] }
	sub FETCH     { $_[0]->[0]++ }
	sub DESTROY   {}

	package main;

	use Test::More tests => 3;

	tie my $foo, "Tie::Inc", 10;

	is_deeply [$foo], [10]; # passes
	is_deeply [$foo], [10]; # fails

> - allows bugs to create false positives *** big no no ***

It is always possible to have a false positive when you have bugs. 

> - is awkward to describe precisely

I don't believe it is.

> - caters for a rather specialised need (in my opinion)

I'd argue this was the other way around.

> - is more complicated to implement

This is irrelevant, and I believe wrong, as the behaviour already
existed and has been changed.

> Our concepts are different. Your definition includes a special case for 
> objects which overload "", mine doesn't and so I'd argue my concept is more 
> "pure". 

No - my concept doesn't involve overloading per se. It involves making
(keeping?) is_deeply() be a straightforward recursive application of is().

> My concept of deep equality is that the 2 things are 
> "indistiguishable", your's is indistinuguishable up to a point. 

What does "indistinguishable" even mean? This is Perl after all...

> If I accept  your concept of deep then I need to accept another one
> which takes numeric overloading into account and possibly other ones
> which handle both and give  priroity to one or the other etc.

If is() doesn't already DTRT thing here, then it should. If it does,
then it should just be used.

> I guess my stance is because I've created a separate module to do deep 
> comparisons which handles these and other special cases in a generalised and 
> extensible way. That's why I'd rather see the more widespread one be 
> absolutely strict and special case free. I'd also like it to be side-effect 
> free which is not guaranteed if it's calling stringify methods.

So using it on anything that's tied should be a fatal error?

>>> 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.

I'm not asking for string equality. I'm asking that is(X, Y) give the
same result as is([X], [Y]). To do otherwise *I* would say is non-intuiutve.

> Because it can and because it says it will. In an earlier email you quoted 
> "descend a structure and apply is()", where did you read this? Here are the 
> docs I have
>            Similar to is(), except that if $this and $that are hash or array
>            references, it does a deep comparison walking each data structure
>            to see if they are equivalent.  

I guess my drugs aren't working well then. Because to me that states the
only "deepness" that it cares about is the position in a list or hash.
Seeing if they are "equivalent" is a matter of ambiguity, but there is
no reason for equivalency here to be in any way different from the
equivalency assured by is().

> It should also say "this test ignores blessings on references". If we make it 
> work with overloading it will have to read "this test ignores blessings on 
> references, except when an object inside $this overloads "" or eq and the 
> corresponding part of $that is a scalar, in which case instead of descending 
> into the object it will use eq to compare it to the scalar". Clear? No.

If you think this needs documented it should be documented at is(), not
is_deeply() as that's where the "problem" if there is one (and I don't
think there is) appears.

> is_deeply("string", $overloaded_object)
> should definitely fail even when your
> is_deeply($overloaded_object, "string")
> would pass.

Absolutely not.

is_deeply (SCALAR, SCALAR) should be able to completely delegate to
is(SCALAR, SCALAR). The docs for is_deeply say NOTHING AT ALL about
testing anything contained within a scalar - only lists and hashes.

> > 	is $fred, "fred";
> > 	is_deeply [$fred], ["fred"];
> > Do you really think that test 1 should pass and test 2 should fail?
> Even if you said
> is $fred, "fred" 
> is_deeply $fred, "fred" 
> 
> I'd say yes, I think they should pass and fail respectively. is() is 
> documented to use eq, is_deeply() is documented to examine the structure of 
> it's arguments, it doesn't say that it examines the structures of it's 
> arguments _sometimes_. 

No - it's documented to walk hashes and lists comparing what's at the
equivalent position in each. 

> It does say it's "similar to is()" but I always took that to mean it
> gives nice diags and it takes the tested value followed by the expected
> value, followed by a name. Maybe I was wrong...

I think you are.

> For me, ideally is_deeply() ans is() would be the same, 

I agree. Your proposals make this not true. (See 15 lines ago!)

> > If so, what about the case where overloaded constants are in operation
> > and my $fred = "fred" creates an object ...
> Anyone lobbing overloaded constants into their tests and expecting everything 
> to work as normal is asking for trouble. You can't have overloaded _and_ 
> behaving exactly as normal, otherwise what's the point?

I'm not talking about doing it explicitly in the test. I'm talking about
testing in the presence of a module that does this. 

>>> What if you had overloaded eq on $ref and were accidentally always 
>>>  returning true? is_deeply($ref, "any string") would be a pass.
>> Yes. But that would be the *correct* behaviour.
> Absolutely not. I said "accidentally" - as in there is a bug. The current 
> is_deeply() would catch this bug. The is_deeply you want would give a false 
> positive.

The computer cannot tell whether or not I did it "accidentally". I may
have done it deliberately. If you can make the tests pass if I did it
deliberately and not if I did it accidentally, then I'll happily accept
the behaviour.

> > If you want to be twiddling with the innards of objects to see if
> > they're the same then you should be using something different.
> is_deeply's docs explicitly say it twiddles the innards, perhaps we need

Where? My docs don't mention objects at all. It only talks about
descending lists and hashes.

Tony