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

On Monday 08 September 2003 10:50, Tony Bowden wrote:
> 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.

I have, although I try to consider it as little as possible. It's an ugly hack 
that's only needed because Perl's builtin types are not objects. 

The important difference is that a tied string is indistinguishable from a 
non-tied string whereas an overloaded reference is very easily 
distinguishable, it responds to a whole load of things that a string won't 
respond to like ->[] or ->{} or ref($thing).

There is no way for any test to spot tied values and so there's no way to do 
data only tests with tied values without the risk of strange breakage, this 
means that data only tests on tied values are of little value, you should be 
explicitly testing the data and behaviour of the underlying object.

This all makes tied values outside the scope of data tests. There's no need to 
add a further class of objects to the list of not-reliably testable things.

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

Absolutely but I did apply the only criteria that I could think of, that is 
does it behave like all the other equalities I can think of: reflexive - a=a; 
transitive - a=b and b=c means a=c; symmetric - a=b means b=a. I think this 
is part of most people's idea of equality.

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

Yes, my point is that my concept is as deep as possible. Do you agree that my 
deep is deeper than yours? If you just take deep to mean the depth you have 
descended in the structure then it certainly is and I can't think of any 
other useful definition. So given that one is a deeper than the other, which 
should be called deep?

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

Destructive as in it doesn't alter the data it's testing.

> > The definition you want
> > - includes behaviour, 
> 
> As does the current version.

No it doesn't, currently it looks only at data and never calls any methods.

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

This is the fundamental difference, I don't think they should do the same 
thing, is() is explicitly documented to use eq, is_deeply is explicitly 
documented to walk _each_ structure it finds.

> > - allows bugs to create false positives *** big no no ***
> 
> It is always possible to have a false positive when you have bugs. 

No, currently is_deeply cannot give a false positive no matter what bugs you 
have (excluding tieing again). If is_deeply says the structures are the same 
then they are the same. That's not to say that the function that returned the 
structre has no bugs but it does definitely say that it worked this time and 
that's the most you can hope for.

If you allow overloading is_deeply will tell you either "it looks exactly like 
the structure you told me to check for, guaranteed, no doubt" _or_ "it 
slipped me a fiver and told me not to look in the bag and that's good enough 
for me".

> > - is awkward to describe precisely
> 
> I don't believe it is.

Please describe it accurately.

> > - caters for a rather specialised need (in my opinion)
> 
> I'd argue this was the other way around.

How many people use string overloading deep inside their objects? How many 
people have is_deeply tests where they include a string but would be happy 
enough if it was string or an overloaded object? Testing should be explicit 
and you should be explicitly testing one of A: a string; Boverloaded object 
striginifying to something; or C anything which stringifies to the correct 
value and you should only be testing for C if you know the object will only 
ever be used in string context and for returns values that means you have 
documented that it must only be used in string context. If you are  not just 
one or the other. As I pointed out, there are many functions that accept 
polymorphic agruments and can behave totally differently with strings vs 
refs. So it is not at all safe to be passing overloaded ref and telling 
people that they really are strings.

> > - is more complicated to implement
> 
> This is irrelevant, and I believe wrong, as the behaviour already
> existed and has been changed.

The behaviour existed but was causing another bug, I fixed that bug and to my 
mind I accidentally fixed another.

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

But it has never been a recursive application of is() and it can never be 
because is() produces a test result every time it is called and is() is not 
eben implemented in Test::More. By involving stringwise equality you _are_  
involving overloading.

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

Indistingushable means I can call f($this) or f($that) and as long as f 
doesn't care about the class of it's argument (because Michael has said class 
isn't included) I'll get the same answer. There is no meaningful difference 
between them.

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

is() does the right thing, it does what it's docs say it will do, it uses eq. 
My point is is_deeply() doesn't say what it will use, so why should it use 
the same as is()?

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

There is no way to spot a tied value so it won't give an error. However if 
there was, I'd probably say yes. Passing tied objects into data tests is just 
dangerous. As I said before you have no way to know how many times a piece of 
data will be accessed during an is_deeply test and you should not need to 
know this either.

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

I think is() was probably the wrong name, given that it is simply a wrapper 
aroung Test::Builder->is_eq it should have been called is_eq but Michael put 
convenience before absolute accuracy which is fine because he documented it.

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

Yes, there is plenty of room for maneuver in the word "equivalent". But there 
is none in "walking each data structure". An overloaded reference has 
structure and given that we're ignoring blessings on all other structures, it 
doesn't make sense sudeenly stop doing that because it's a very specific type 
of blessing.

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

It _is_ documented in is(), it says it uses eq.

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

The second argument is the expected value, if the expected value is an 
overloaded object then nothing less than an overloaded object should be 
acceptable as the tested value, otherwise the test is pointless.

The tested value should have no say whatsoever in how the test is carried out, 
just like students don't get to write "yes I know the answer, you can take my 
word for it" on an exam paper.

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

Firstly the docs are wrong there because it does descend into scalar refs and 
I think that's implied by "deeply", "lists and hashes" was not supposed to be 
an exhaustive list of the things it will descend into.

Secondly I'm not sure what you mean about descending into scalars vs lists and 
hashes. If the scalar is a simple scalar it's not possible to descend into it 
but an overloaded object is a million miles from being a simple scalar, in 
fact it is almost always a ref to an array or a hash and therefore descent is 
appropriate.

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

What was $fred in that example? It was an object and therefore it was a ref to 
a hash or an array. Yes it could have been a blessed globref, scalarref or 
regexpref, the same arguments apply.

> > For me, ideally is_deeply() ans is() would be the same, 
> 
> I agree. Your proposals make this not true. (See 15 lines ago!)

I'm afraid you don't agree because I phrased that badly. What I should have 
said is... Ideally is() would have been a very strict deep comparison to 
determine if 2 things are exactly the same. What we currently call is() would 
be called is_eq() or is_stringly(), there would have been no is_deeply().

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

I'm not familiar with it at all but if it's in happening in the test script 
then it must have been requested by the test script. If it's happening 
without the test script asking for it then someone has written a rather 
naughty module that will break a lot more than just this script.

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

The purpose of the test is not to determine whether it is deliberate, it's to 
determine whether it's happening or not. When the current is_deeply() say 
"OK" or "NOT OK" you know it's true. When your is_deeply() says "OK" it could 
be right or you could have a bug somewhere else. The only way to be sure is 
to also test every single reference in the data to make sure that it's not 
got a dodgy eq method.

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

What are objects except blessed arrays and hashes? In this thread

http://www.mail-archive.com/[email protected]/msg01679.html

Michael explains that is_deeply() ignores blessings to make it easy to test 
objects, so treating objects as just arrays and hashes is definitely in the 
plan. He also said that having it be smart about how it tests certain things 
depending on the expected value is undesirable, so I was a little surprised 
when he changed his mind,

F