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

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
- intuitive: like the definitions of other kinds of equality it is symmetric, 
reflexive and transitive
- as deep as possible, something shouldn't claim to be deep but change it's 
mind sometimes.
- non-destructive
- strict
- leaves no room for confusion or doubt about what it does

The definition you want

- includes behaviour, sometimes and for some reason "" is more important than 
0+ overloading
- is neither symmetric nor transitive and so is unlike other types of equality
- is not always as deep as it could be
- can possibly alter the data
- allows bugs to create false positives *** big no no ***
- is awkward to describe precisely
- caters for a rather specialised need (in my opinion)
- is more complicated to implement

On Saturday 06 September 2003 12:44, Tony Bowden wrote:
> I still maintain that your concept of 'deep' here is wrong.

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". My concept of deep equality is that the 2 things are 
"indistiguishable", your's is indistinuguishable up to a point. 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.

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.

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

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.  If the two structures are differ-
           ent, it will display the place where they start differing.

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.

I have not deliberately obfuscated this (much ;-). Try and make it more clear 
but remember that is_deeply() with overloading _must not_ be symmetric in 
$this and $that.

Why must it be asymmetric? Because

is_deeply("string", $overloaded_object)

should definitely fail even when your

is_deeply($overloaded_object, "string")

would pass.

> 	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_. It doesn't say anythig about using eq either. 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...

For me, ideally is_deeply() ans is() would be the same, I think it's a little 
confusing that

is($ref, $ref."")

is a pass.

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

> > 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 worst thing you can do in testing is give a false positive. Tests should 
be as strict and predictable as possible.

> 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

is_mostly_deeply() and
is_really_deeply_honestly_it_is_guvnor()

;-)

> > Like I said, I've asked Michael Schwern for his take.
> 
> Like I said, he's given it:

Sorry, I was using internet cafes for the last few days, so with the help of 
several hundred "Re: Your details" emails I missed it.

> 	> is_deeply [ State->columns('Primary') ] => [qw/name/],
> 	>   'State Primary:' . join ", ", State->columns('Primary');
> 
> 	This should work.  The test should not ignore stringification otherwise
> 	you can't do black box testing using is_deeply().

The test ignores numification, why shouldn't it ignore stringification?

In this case, is_deeply is unnecessary, you're just using it as a convenient 
way to test a function in array context, at the risk of going on about it

cmp_deeply([ State->columns('Primary') ], [str("name")])

would do exactly what you want and it makes the stringification explicit.

As for the black box stuff, I don't follow this at all. is_deeply() already 
prevents black-box testing insofar as it requires you to fully predict the 
data structure that will be returned. The closest you can get to black box 
testing using is_deeply is

$black_box = Black::Box::some_function();

is_deeply(
  $thing,
  {
    key => $black_box
  }
);

which works perfectly with the current is_deeply.

Something else Michael said was

> Put another way, the Class::DBI API says that columns() returns a list of
> column names as strings.  There's nothing about them being objects.  When
> writing the test we should not have to be aware that they are really
> objects with stringification overloaded.

which I found a bit strange. If the API says strings and nothing about 
objects, then it should give strings, not objects. The API is a contract and 
if I'm told I will get strings then I have every right to use code that 
explictly _depends_ on them being strings and not objects. There are many 
functions that are polymorphic in their arguments and behave differently when 
given a string or a ref

So I can do

poly_fn("string");

or

poly_fn({Type => "special", Name => "string"});

however if I do

poly_fn(State->columns('Primary'));

I'm in for a surprise and possibly a long debugging session until I realise 
that the API docs are not quite right. The API docs should make it clear that 
you get something which stringifies, that way it's free to give a real string 
or a fake one and it's caveat usor,

F