Create a check "nonblank"

[petdance]

In the old system, I have many tests that are of the form

is_nonblank( $foo->{name} );

which is effectively

ok( !ref($foo->{name}) && length($foo->{name}) > 0 );

And we should be able to do

is( $foo, hash { field name => nonblank } );

[schwern]

I think this is a good idea, but deciding what "non-blank" is will need some discussion.

First issue is the !ref check. That should be removed otherwise string and numeric overloaded objects will be "blank". For example.

use Test::More;
use URI;
my $foo = { name => URI->new("http://example.com") };
ok( !ref($foo->{name}) && length($foo->{name}) > 0 ) || diag $foo->{name};
done_testing;

In general, implicitly excluding objects or references from checks will lead to confusing results and pierce the encapsulation of overloaded objects. It could be argued that non-blessed references should be excluded, or even blessed references which are not string or numeric overloaded, but not simply all references.

I say to keep it simple and to let the caller decide whether they want to exclude references.

[schwern]

Next is the term "blank". What does it mean? The code says it's something that has a length. What about " "? That has a length, but it's arguably "blank".

It would be clearer to call this empty_string defined as "it stringifies to """. Then you can use !empty_string to check that something will stringify to something other than an empty string. This clears up all sorts of ambiguities.

These match empty_string:

• undef
• ""
• URI->new("")

These do not:

• 0
• []
• " "
• URI->new("http://example.com")

[exodist]

I agree with @schwern here, if the blank/non-blank were to be a core part of Test2::Suite it would need to look like this to be properly generic, and meet expectations.

That said, I am guessing this does not help you with your use case as much as you would like. However you could then wrap this with a couple other exisitng tools to create your_not_blank() to suppliment it in your own test suite's tools.

[petdance]

The !ref is so that you won't pass when passing [...] or {...} or an object, which is part of the problem it was designed to solve. You get a ref back from a sub thinking you're going to get a string, and my is_nonblank() catches that.

The !empty_string makes sense, but I'm wary of allowing URI->new("") passing. Or does it pass only because it string overloads, and another object wouldn't?

[petdance]

I'm also thinking that it should be string_empty, not empty_string, because there could be string_all_caps or string_url or any number of string_whatever coming down the pipe.

[exodist]

Something with string in the name should stringify the argument it is given, so if URI->new("") stringifies to "" then it should pass as an empty string.

[exodist]

I think empty_string is a better name than string_empty. The other thingsd you mention may come to exist, but seem to specialized for core.

[petdance]

I take it back, I think this is confusing. field name => !empty_string looks like it would match, say, undef, or a hashref, or 14, or anything that is not ''.

[schwern]

@petdance URI->new is a good example because it masquerades as a string. You can pass it around as a string and never know it's an object. That's part of the point of string overloading. So at minimum the test would have to detect if the ref is a string overloaded object (including fallbacks) and let it through. It's not hard, but it adds a bunch of complexity.

As for whether [] and non-string overloaded objects should be an exception... if you use them as strings they stringify to non-empty strings. They're non-empty strings. So when you say this is confusing...

I think this is confusing. field name => !empty_string looks like it would match, say, undef, or a hashref, or anything that is not an ''.

I say "works as designed". It keeps it simple. The check does one thing, it checks if a thing will stringify as an empty string or not. This simple definition avoids having to make a bunch of judgement calls and edge cases and impose a philosophy about objects (ie. treating refs and objects as special things). The check only cares about what it stringifies to.

If you want to also check that it's not a reference, make that explicit. field name => !empty_string && !ref. I don't know if && and || are overloaded for checks, but it sure would be cool if they were!

[exodist]

&& is not overloaded, but it easily could be using the set checks which let you check that all checks within them match, or that only one does, or that any one does... @schwern can you make a ticket for fleshing out that feature?

[autarch]

I think calling it non_empty_string would be best. It's ugly but when I google "non-empty string" I get a decent number of hits, suggesting it has some precedent in terms of how devs talk about these things. Also see https://metacpan.org/pod/MooseX::Types::Common::String for another usage of the term.

[autarch]

Also, I think it's incredibly uncommon to check that something is an empty string, whereas checking for non-emptiness is extremely common. I use that MX NonEmptyStr type all the time in my code.

[petdance]

If you want to also check that it's not a reference, make that explicit.

The whole point is to not have to make it explicit. The point is to have a string that is not a reference.

Let me describe the use case. I know it probably doesn't fit the model you have in mind.

Right now I might have code like this:

my $user = get_user();

isa_ok( $user, 'My::User' );

my $name = $user->name;
ok( !ref($name) );    # If I get a reference or object back, that's an error.
ok( length($name) > 0 );  # It can't be blank

my $accounts = $user->accounts;
ok( ref($accounts) eq 'ARRAY' );  # Have to get an array back
ok( @$accounts > 0 ); # Has to be a nonempty array

my $rank = $user->sales_rank;
ok( !ref($rank) ); # Has to not an object of any kind.
like( $rank, qr/$\d+$/ ); # Has to be an integer
ok( $rank > 0 );  # Has to be greater than 0

These are so common I have wrapped them up into

is_nonblank( $user->name );
is_nonempty_array( $user->accounts );
is_positive_integer( $user->sales_rank );

For me, the promise of Test2 is the ability to do this:

is( get_user(),
    object {
        obj_type => 'My::User';
        field name => nonempty_string;
        field accounts => nonempty_array;
        field sales_rank => positive_integer;
        end;
    }
);

[schwern]

@petdance

I know it probably doesn't fit the model you have in mind.

Exactly the problem. Not just with my model, but with the probably half a dozen different ways people think about refs and objects in Perl floating around out there. Which is to say that your use case is totally valid; the question is how to solve it without invalidating the other common use cases.

Adding extra implicit caveats to checks restricts how they can be used. If empty_string means "its an empty string and it's not a reference" it cannot be used by someone like me who doesn't differentiate between "real" strings and things that act like strings.

This is why I advocate simplicity, meaning few moving parts. A check that does one thing has maximum utility for the maximum number of situations and more specialized and complex/compound checks can be built on it.

I think #75 resolves this problem between flexible simplicity and specific complexity by making multiple checks easy to combine without having to write a plethora of new combined checks. Hopefully we can write your example as...

is( get_user(),
    object {
        obj_type => 'My::User';
        field name => !empty_string;
        field accounts => !empty_array;
        field sales_rank => positive_integer;
    }
);

[schwern]

As for non_empty_string that's just begging for double negatives. We have ! now.

[petdance]

the question is how to solve it without invalidating the other common use cases.

Agreed 100%. Components are good.

Hopefully we can write your example as...

is( get_user(),
    object {
        obj_type => 'My::User';
        field name => !empty_string;
        field accounts => !empty_array;
        field sales_rank => positive_integer;
    }
);

This goes back to what I said above: field name => !empty_string looks like it would match, say, undef, or a hashref, or 14, or anything that is not ''. !empty_array looks like it would match a hashref, or a 14, or undef or "Blah blah".

[schwern]

@petdance Good one, the problem is much clearer with empty_array. Yeah, empty_blah should mean "it (is|behaves like) a blah and it is also an empty blah". The "(is|behaves like)" is still there because of the overloading problem: @{} (ie. dereferencing an array) can be overloaded.

Ok, here's my summary of the approaches.

Invert the check to populated_blah.

This fixes is {}, populated_array; since a hash references is not an array with things in it, but it still leaves is {}, !populated_array; ambiguous.

Anything that isn't a "real" array/hash/number/string fails.

This breaks encapsulation of overloaded objects, very common for stringification. And with so many things in Perl automatically stringifying it's particularly weird for empty_string.

Only things which behave like an empty blah pass.

I'd argued that people should say is {}, string + empty_string or something, but this becomes nonsense when talking about arrays and hashes. is {}, empty_array should not pass and all the empty_blah checks should behave similarly.

All references fail.

This breaks encapsulation of overloaded objects.

All non-blessed references fail.

Not all objects are overloaded.

First introspect a reference to see if it will behave like a blah.

Like return FAIL if ref $thing && !overload::Method($thing, q[""]). This is a bunch of internal complexity and I'm not convinced we even know all the ways people can mess with the behavior of references. There's a better option...

Use the behavior of references.

empty_blah would try to use it as a blah wrapped in a try block. If it works, great! If it doesn't, it fails. I like this over introspecting the thing to see if it implements overloading because we don't have to worry about all the ways a Perl reference might behave differently. It should be documented as "it behaves like a blah and it is an empty blah" ("behaves" as opposed to "is").

Buuut with references stringifying this brings us back to whether is {}, empty_string should pass or fail. Consistency and behavior of the code says pass. User expectations say fail.

Use the behavior of references, but string and number checks first look for string/number overloading.

This is just like before with special exceptions for all string and numeric checks. They all check for string and numeric overloading (be sure to take into account fallback!) before considering an object as a string.

Yes, numbers. Did you know references can be used as numbers?

$ perl -wle 'print 0 + {}'
140668399801960

Yup, cmp_ok {}, ">", 0; will pass. That's just a weird quirk that I've never heard being encountered. The DSL is going to bring out this problem. Users probably won't expect is {}, positive_integer to pass.

Fortunately checking for overloading is safe and easy, Test::More has been doing it for a long time.

I think this is the best thing we can do. It balances consistency with user expectations.

Thoughts?

[schwern]

Oh 💩, another edge case. What do we do about this?

my $obj = bless {}, "Some::Class";
is $obj, empty_hash;

Should a blessed hash reference be treated like a hash?

As much as I'd like to say "no, don't peek inside objects!" I realize a large bulk of the Perl 5 community is against me on this one. So I'll concede that should pass.

If you don't want it to pass, write is $obj, !object + empty_hash or something.

Whew! Thoughts?

[exodist]

My thought is that if the name includes the structure type, 'array', 'hash', 'object', etc. That the check should verify that the thing being checked is of that type, or can pretend to be that type via overloading (or is that type but blessed). This includes the negated ones. so !populated_array still implies that it must be an array.

Negation negates the state of the structure, but not the type. This is consistent with how I have been trying to do all the checks.

populated_array and !populated_array should both fail if the thing they are checking is not an array.

This is why checks have both verify() and delta() verify() checks that the thing is the expected type (array), delta() then returns the differences of the contents.

populated_array should have a verify() that returns true/false based on the type of the thing being checked. It's delta() should be run only when verify() passes (that is baked in already) and return differences between got and want knowing they are at least the same type.

[exodist]

If an object is a hash, then populated_hash and !populated_hash should both have their verify step pass, cause the thing they got can be used as a hash.

[exodist]

Fact is there is ambiguity, we cannot get rid of ambiguity completely, but what we can do is always treat the ambiguous cases consistently, and that means a check against a structure must verify it got something of that structure, even if it's checks inside the structure have been negated via !.

This should be documented as the "consistent" thing to do, and anything we have already that does nto conform to this should be fixed (I am pretty sure everything conforms)

[schwern]

@exodist You said "type" and "is" a lot, not "behaves". This is important. Do you mean "behaves like a hash/array/string/number" or do you mean "is a hash/array/string/number"?

[exodist]

if it can sanely be treated like an array (is an array, is tied as an array, overloaded to be an array, etc) then verify() should pass on anything that implies it works on an array, even when negated.

[exodist]

to be clear "behaves like an array"

[exodist]

Thinking about it, the array and hash checks as they stand will fail on anything that is not reftype(thing) eq 'ARRAY' they should be fixed to let you use anything that can behave as an array/hash/etc.

[autarch]

I note that Ref::Util provides is_plain_hashref, is_blessed_hashref, and is_hashref because this is such a mess. FWIW, I'm strongly in favor of defaulting to not peeking inside blessed structures, on the theory that if I blessed it I meant to treat it as an object, not a data structure.

Also, to go back to overloading a bit ... this is a huge mess with string & number overloading, since all Perl types are capable of acting like strings or numbers, but only in the most useless way possible. To actually check whether something acts like a number in Perl is really painful and requires you to complete violate overloading encapsulation. See http://blog.urth.org/2010/10/16/whats-wrong-with-perl-5s-overloading/ for a blog post I wrote about this years ago, as well as Specio's type check's for these things for more painful details.

[exodist]

The main difference here is we are not "guessing" in any way. The check on the right hand side dictates what should be in the left hand side (the got structure). So if you do number(5) on the right hand side it will only pass if the left is == 5 when treated as a number. Similar for string("foo"), passes only when the left hand side is "foo" when treated as a string.

In neither case does it inspect internals, or try to guess if it is a number vs string vs ref, it just treats it as what it expects, and fails if that produces no match, passes if it produces a match.

We must not go down the smartmatch road to hell by trying to guess these things.

Carrying this forward, array { ... } should pass when the left hand side can be treated as an array (ie, using $rhs->[0] or @{$rhs} works) and all the indexes match their checks.

If the check contains the word array it implies that the check must be used against an array, or something that behaves as an array. IF the thing it gets does not behave as an array it should fail, and not get to the nested checks, negated or not.

[petdance]

Yes, except that ref and blank are not checks right now.

I know, I'm talking high-level here.

What if I did it as

sub nonblank_string {
    check_set( D, !ref, !blank );
}

is( $user,
    object {
        field name => nonblank_string;
    }
);

That's really the more likely use anyway, that users will set up their own meta-checks, like maybe

sub isbn10 {
    check_set( D, !ref, match qr/^\d{9}[\dX]$/ );
}

[exodist]

That will still have the wrong line numbers, it will report to being inside your sub. I anticipated this in the todo list above, I will write a tool to make it easy. Probably do not have time to write the tool today though.

[exodist]

to be clear, here is what I have in mind:

# This will define the nonblank() sub in the current namespace
compose_check nonblank => (D, !ref, !blank);

# Then use it.
is($x, nonblank());

Perhapse also this:

use Ndn::Tools::Compare::Compose(
    nonblank => [D, !ref, !blank],
);

so that people can define them at BEGIN time easily

also, I think compose_check should define the sub in void context, but return an anonymous sub in non-void context.

[petdance]

So long as I can call compose_check nonblank => (D, !ref, !blank); in My::Local::Test2::Bundle and have it propagate out to calling *.t files. That's what it looks like you're talking about in Ndn::Tools::Compare::Compose?

I just want to be able to

use My::Local::Test2::Bundle;

is( $x, nonblank() );

[exodist]

You will be able to.

[exodist]

oops, did not intend to close.

[schwern]

I'm cool with the policy list exodist posted, with one major exception.

_Treating default reference numification/stringification as a number/string is useless and error prone._

I'm with @autarch and @petdance on that one now. Any "string" and "number" type checks should reject default reference numificaiton and stringification (ie. 140507464357480 and ARRAY(0x7ff829005668)) because it's a useless feature. I think this special case is worth the effort and complexity, fortunately encapsulated inside Test2, because it reflects user expectations and testing needs. For every time someone might want is {}, !empty_string to pass there's easily 100 or 1000 or more where it should not. If we require is $foo, real_string + !empty_string 99 out of 100 times people will forget since 99 out of 100 times is $foo, !empty_string "works" and the mistake is silent.

So long as we do it consistently I don't think anyone will even notice because it Just Works.

Go ahead and look through your existing tests and mentally count which ones really want ARRAY(0x7ff829005668) to pass a string test. Or how many of your cmp_ok tests would "pass" if fed a reference.

Exceptional cases should be exceptional. If someone really wants to test default reference numification and stringification, they can explicitly numify or stringify it.

Or hey, checks are functions. Add an option. is {}, !empty_array( allow_bare_refs => 1 );

[rjbs]

As for non_empty_string that's just begging for double negatives. We have ! now.

Did this get addressed and I missed it? It sounds like the current plan is:

empty_string means "a string, and it is empty."

and

!empty_string means "a string, which is not empty."

Which means that every test is defining the semantics of its own negation, and that these two lines are not equivalent:

not( empty_string->matches( $x ));
( ! empty_string)->matches( $x );

This makes me a lot more unhappy than "double negation." If we have two clearly-defined tests, each of which can be negated through a generic mechanism that means "does not pass this," then it's clear. If every assertion has two meanings, I am sure I will end up pretty confused in short order.

[schwern]

@rjbs Does #73 (comment) clear up why the type checks are necessary and why they don't also negate?

Looking at it another way, we're negating the "empty" part, not the "type check" part because that would be nonsense. And yes, this is different from negating the result. We can get away with this because it's a DSL and not intended to be used as code with explicit method calls like you have.

This is one of those things that only becomes a problem when you think about it too much. :)

[schwern]

@rjbs To put it another way, we intend !empty_box to scan as "I want a box that's not empty". The literal interpretation of "I want something which isn't an empty box" is awkward. If you said !empty_box and I handed you a cat, you wouldn't be happy (neither would the cat because they don't have an empty box to play in). Only a robot would be happy.

Maybe what we need is a more generic empty check! Then we can write:

is $thing, string + empty;

I'm liking that! The bare reference string/number problem is easier to resolve. We can offer string (anything that behaves like a string except default ref stringification), real_string (it's a non-reference scalar), and maybe any_string (which is basically everything but undef).

This also avoids a proliferation of empty_blah functions, at least in the Test2 core. The different empty checks for various types would be internal to Test2 and hidden from the user.

OH! Maybe take a page from array and hash!

is $thing, string { empty };

I think that's it! Then there's no ambiguity about what this means.

is $thing, string { !empty };

[exodist]

@schwern, I agree with what you say. we can make default ref string/numberising to be an exception.

[petdance]

Plus, if someone wants to check stringification of their object, they can explicitly put the object in quotes to get the stringification.

my $obj = SomeObj->new;
is( $obj, empty_string ); # Fails because $obj is a reference.
is( "$obj", empty_string ); # Passes because the user has stringified it explicitly.

[rjbs]

they can explicitly put the object in quotes to get the stringification

I strongly object to this suggestion. If my input is:

$input = { a => { b => [ ..., $c ] } };

I want to be able to write:

is($input, $complicated_test);

and not

is("$input->{a}{b}[9]", $test1);
is("$input->{a}{z}{w}", $test2);

and so on. It is the test specification that should say what it means.

[2shortplanks]

I think the real problem we have here is the classic no-one understands what overloaded negation actually means - at least not consistently with what other people think. This is why I think the magical aspect of it is just a really bad idea and we should avoid it entirely. No matter what we decide it means half the people will think it means the other thing and that's how we end up with lots of buggy tests. The less magic in tests the better.

I'm not entirely sure why string, empty_string and non_empty_string are off the table. I find them significantly clearer about what they actually mean.

Maybe we could add some other syntactic sugar meaning 'something that doesn't match this condition' and stay away from ! there too. I'd recommend a keyword anything_but. with clever uses of prototypes this would allow you to write anything_but non_empty_string.

[exodist]

I feel like this thread is getting crazy. @rjbs I don't think anyone is proposing what you are against, though the wording @petdance used does seem to imply that way.

The only thing being proposed is that plain refs without string overloading, ie 'hash(1244)' is almost never what you want. By that a string check on the left that only cares that it is given a string (but does not care what is in it) should not pass if it gets a ref that has no string overloading. in this case you can still do the uncommon case of actually wanting 'hash(1234)' by quoting thr thing on the left.

[petdance]

I think @rjbs and I are talking about two different things, and I don't disagree with the things he's talking about at all.

To clarify, Rik, what I was saying is that we want this:

is( $foo, nonblank_string );

to verify that $foo is \an actual string, that is not an object that just happens to stringify to something.

Right now, in Test::More, if you say

$foo = {};
isnt( $foo, '' );

the test will pass because $foo stringifies to a HASH(0xBLAHBLAH). What we want to say is that in Test2, this test will fail so as to catch the mistake.

$foo = {};
is( $foo, nonblank_string ); # Fails because $foo is not a string, it's a reference.

And if we do want to explicitly check for overloaded stringiness, we can do so like so:

$foo = URI->new( 'http://blahblah.com' );
is( $foo, nonblank_string ); # Fails because $foo is a reference
is( "$foo", nonblank_string ); # Passes because the stringified $foo is a string.

[exodist]

I think the following:

1. This is getting too magical
2. We already have too many DSL functions
3. There is not enough consistency of the DSL functions

I think we need to move to the less is more approach. I think the dsl should have only a handful of subs:

is(
    $structure,
    st Hash => sub {
        my $st = shift;
        # or
        my $st = st_this;

        stc key1 => 'val1';
        stc key2 => 'val2';

        stc nested => st Array => { sta empty => 1 };

        stc non_empty_string => st String => { sta empty => 0 };

        stc price => st Number => 12; 

        sta 'end'; # Or to be more explicit:
        sta end => 1; # or stp etc => 1,
    },  
    "name"
);

st - define a structure
st Type Val
st Type \&Builder
to define a Test2::Compare::Type check

stc - structure component;
stc key => 'val' # like 'field'
stc idx => 'val' # for arrays (like item with an index)
stc 'val' # for arrays (like item with no idx)
stc $val # String/number/etc.
to define components inside the structure

stcs - structure values
stcs ...; # Specify several components at once

sta - structure attributes
sta 'attr' - toggle attribute on
sta attr => $val - set attribute to value
This is essentially calling a method on the check when used in a builder to toggle attributes

stp - structure property
stp what => val
This would be like a 'prop' to check properties of the thing you got.

Other subs can be added as shortcuts for some common cases, in fact all current dsl subs can be implemented with these.

This interface still allows for builders (the kind that take subs) while allowing for those that do not (like string() and number()) It is a bit more verbose, but it leads to more consistency, and less need to know the differences between similar things.

Negation of a structure check (all checks are now structure checks, just that string and number are not deep structures) will still enforce an instance of the structure, but negate the contents of the structure.

We cannot negate something that isn't assigned. Schwern wanted !empty called in a void context, I could see this being achievable with some magic, but I really do not want that kind of magic. I think 'sta empty => $trival' where $trival can be true, false, or undefined (allow empty, disallow empty, don't care) is a but more sane.

stc foo => sub { sta empty => 0 } to check for a non-empty string. Then for petdance he can write a shortcut to contain that with less verbosity.

This solves the array { ... } vs string(...) inconsistency as well as both are:

st Array => \@array;
st Array => sub { ... };
st String => "...";
st String => sub { ... };

consistent! no prototypes, no magic!

Final point to address, petdance implied we would also change isnt(). That needs to be discussed. Currently isn't is a full fledged negation, it passes if any of the checks fail. I have been thinking isnt() is it is currently defined is useless for all but shallow checks isnt(undef, 'not_undef'), which is frankly better written as is(undef, undef)

[exodist]

I am going to create a milestone for all the various issues here, and split the thing from this ticket into several tickets inside the milestone. I think there are too many discussions here.

[exodist]

https://github.com/Test-More/Test2-Suite/milestone/1 created.

[petdance]

Sorry, I made a mistake up above. I didn't mean to say that we would change anything about isnt. I used isnt by mistake.

Here's corrected version (and I corrected above, too)

$foo = URI->new( 'http://blahblah.com' );
is( $foo, nonblank_string ); # Fails because $foo is a reference
is( "$foo", nonblank_string ); # Passes because the stringified $foo is a string.

[exodist]

I think I captured all the issues fairly well in the milestone.

A combination of things mean I probably cannot work much on it this week, hopefully the tickets make a few things clear. It should be possible for others to work many of them if anyone feels like it.

[exodist]

I am going to close this ticket in favor of the tickets that came from it. Many of them still reference this ticket making the history easy to find.