Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for meta-checks in hash, bag, and array builders #177

Merged
merged 1 commit into from Mar 16, 2019

Conversation

@jjatria
Copy link
Contributor

commented Mar 15, 2019

This is an attempt at addressing the issue in #131.

I'm not sure if this is a desired feature or not, since the issue has been around since 2017 and is still open with no comments. But I agree with the poster in that it seems like something that should be possible, and it certainly seems clearer to write

is [1], array { prop size => 1; etc; };

than

is [1], array { item 0 => E; item 1 => DNE; end; };
@exodist

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

The original idea was that if you just want meta checks use the meta builder:
is([1], meta { prop size => 1 });
If you want to check both meta and regular then you have 2 options

Option 1: do 2 checks:

is([1], meta { prop size => 1 });
is([1], array { item 1; });

Option 2, nest an array check under meta:
is([1], meta { prop size => 1; prop this => array { item 1 })

That is all of course assuming this is a trivial example of something more complex. If you simply want to assert the value of some elements and insure there are no extra elements beyond it you have 'end':
is([1], array { item 1; end }) which will check the first item to make sure it is '1', then make sure there are no additional items in the array. This also works for bag and hash.

@exodist

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Now, I am open to arguments in favor of this feature, but I would like a compelling case where the options above are a burden compared to inline meta checks.

@jjatria

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

To be honest, I had never thought of making those inline meta checks like that. It might be a good idea to add examples like that to the documentation (or highlight it if it's there already) to illustrate how this feature is intended to be used. If I had seen it, then maybe I wouldn't have felt I wanted this feature in the first place. 🙂

That said, I would say the greatest benefit of the Test2 comparison methods is their versatility, clarity, and conciseness. The only argument I can give for this feature (although I'm not sure you'll find it compelling enough) is that not having it seems to me to diminish all three of them.

Being able to write

is \@array, array { prop size => $expected; ...; end; };

seems to me to be clearer and more concise than either of these two alternatives:

is \@array, array { item $expected - 1 => E; ...; end; };
is \@array, meta { prop size => $expected; prop this => array { ...; end; }; };

Not to mention that the first one is more brittle (since it needs to be manually kept at the end, because items need to be added in order) and (I just noticed) would not work with bag in a confusing way:

is [1], bag   { item 3 => E; end; };   # ok
is [1], array { item 3 => E; end; };   # not ok

When I started reading through the Test2 docs and first came across the prop check, I put it down as a feature I'd like to use. But every time I tried using it where it felt natural (with bag, array or hash checks), I got an error. The one time I tried using a meta check I did the other way around:

is \@array, array { ...; meta { ... } };

and I gave up when that didn't work.

I would actually put this the other way around: what is the rationale for making these check builders not support this feature? I think maybe even just making that clearer would be enough for me to close this.

@exodist

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I think your argument is compelling. I think it is more clear to allow meta checks inside hashes, arrays, and bags.

When I think back to why they were not included I think it was order of invention. I think hash {} and array {} were invented first, then object {}, and then meta {} was invented specifically to solve some problems with object {} and that is why object {} allows meta-checks. hash {} and array {} were simply never given the same treatment. bag {} was created later using array {} as a template, so meta {} inclusion was probably never really an idea that happened.

Let me take a look at the commit and see if I have any technical things that need to be addressed, if there are no code issues I will merge this.

@exodist

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

It is a pretty simple commit. My only complaint is that the new functionality is not tested. Can you please add tests for bag, array, and hash that verify the new behavior? Each module should have a module specific test file under the t/ directory where you can add new assertions.

@jjatria jjatria force-pushed the jjatria:array-meta branch from 08feb8c to 75ebc7b Mar 16, 2019

@jjatria

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

I've added some tests copied inspired by the existing ones.

I thought maybe the code could be made less repetitive, but that seems to me to be a larger change, so I'll let you be the judge. I looked at the documentation to see if anything needed changing, but since this was not mentioned explicitly, I didn't change anything.

Do let me know if there's anything else you'd like changed!

@exodist

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Great, can you add one more set of tests to Compare.t that verifies the 'prop' function works when called inside a bag/array/hash? Once that is added I think I will be ready to merge this (but I am about to go afk for a while, so it will likely be tomorrow)

@jjatria jjatria force-pushed the jjatria:array-meta branch from 75ebc7b to e9d071e Mar 16, 2019

@jjatria

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

I've added some positive and negative tests. Let me know how that looks.
No rush on the merge, by the way. This has already been really fast!

@exodist exodist merged commit 6ce5f28 into Test-More:master Mar 16, 2019

@exodist

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Uploaded a new release to cpan.

@jjatria jjatria deleted the jjatria:array-meta branch Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.