Add isNotEmpty to core_matchers.

Add isNotEmpty to core_matchers.

BUG=https://code.google.com/p/dart/issues/detail?id=21562
R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42119

[blackhc, 2014-11-27 14:23:42]

blackhc@google.com changed reviewers:
+ kevmoo@google.com, lrn@google.com

[blackhc, 2014-11-27 14:23:43]

Please change the reviewers appropriately :) I haven't used depot_tools before
etc.
Thanks,
 Andreas

PS: In a different CL, I'll also look into changing the behavior of
isEmpty/isNotEmpty to fail differently when null or an instance of a wrong type
is passed to make it work with isNot.

[Lasse Reichstein Nielsen, 2014-11-27 15:22:47]

How about testing with maps, and a wider variety of iterables (List, Set, Queue,
and some map/where results).

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
File pkg/matcher/lib/src/core_matchers.dart (right):

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:20: } else if (item is String) {
Consider merging String into the previous "if"-condition here too.

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:30: // (including collections).
Second line isn't part of dartdoc-comment.

Document that this is based on the `isNotEmpty` property of the item, so if
something implements inconsistent `isEmpty`, `isNotEmpty` and `length`, it's
clear what is being tested.

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/test/matchers_mi...
File pkg/matcher/test/matchers_minified_test.dart (right):

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/test/matchers_mi...
pkg/matcher/test/matchers_minified_test.dart:117: matches(r"Expected: non-empty
+Actual: " + _MINIFIED_NAME + r":\[\]"));
Long line.
The first part of the string doesn't have to be raw, so you could interpolate
the minified name into that.

[Lasse Reichstein Nielsen, 2014-11-27 15:27:03]

lrn@google.com changed reviewers:
+ nweiz@google.com

[Lasse Reichstein Nielsen, 2014-11-27 15:27:03]

Adding someone with more experience with the unittest package.

[dskloet, 2014-11-27 15:27:52]

dskloet@google.com changed reviewers:
+ dskloet@google.com

[dskloet, 2014-11-27 15:27:53]

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
File pkg/matcher/lib/src/core_matchers.dart (right):

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:20: } else if (item is String) {
On 2014/11/27 15:22:46, Lasse Reichstein Nielsen wrote:
> Consider merging String into the previous "if"-condition here too.

It could just be:

return (item is Map || item is Iterable || item is String) && item.isEmpty;

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:36: if (item is Map || item is Iterable
|| item is String) {
return (item is Map || item is Iterable || item is String) && item.isNotEmpty;

[blackhc, 2014-11-28 16:12:30]

Thanks for the review!

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
File pkg/matcher/lib/src/core_matchers.dart (right):

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:20: } else if (item is String) {
On 2014/11/27 15:22:46, Lasse Reichstein Nielsen wrote:
> Consider merging String into the previous "if"-condition here too.

Done.

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:30: // (including collections).
On 2014/11/27 15:22:46, Lasse Reichstein Nielsen wrote:
> Second line isn't part of dartdoc-comment.
> 
> Document that this is based on the `isNotEmpty` property of the item, so if
> something implements inconsistent `isEmpty`, `isNotEmpty` and `length`, it's
> clear what is being tested.

Done.

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/lib/src/core_mat...
pkg/matcher/lib/src/core_matchers.dart:36: if (item is Map || item is Iterable
|| item is String) {
On 2014/11/27 15:27:53, dskloet wrote:
> return (item is Map || item is Iterable || item is String) && item.isNotEmpty;

Done.

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/test/matchers_mi...
File pkg/matcher/test/matchers_minified_test.dart (right):

https://codereview.chromium.org/763783003/diff/1/pkg/matcher/test/matchers_mi...
pkg/matcher/test/matchers_minified_test.dart:117: matches(r"Expected: non-empty
+Actual: " + _MINIFIED_NAME + r":\[\]"));
On 2014/11/27 15:22:46, Lasse Reichstein Nielsen wrote:
> Long line.
> The first part of the string doesn't have to be raw, so you could interpolate
> the minified name into that.

There are 10 matches in the unit tests for this kind of string concatenation and
I think it would be better to change them all at once instead of introducing an
inconsistency here. I'm not sure this CL is the best place to do that. What do
you think?

[nweiz, 2014-12-01 22:18:07]

lgtm

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/lib/src/core...
File pkg/matcher/lib/src/core_matchers.dart (right):

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/lib/src/core...
pkg/matcher/lib/src/core_matchers.dart:29: bool matches(item, Map matchState) {
Nit: empty lines between members.

The existing code here is quite old and doesn't always follow the style guide,
but new additions should do so.

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/test/matcher...
File pkg/matcher/test/matchers_unminified_test.dart (right):

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/test/matcher...
pkg/matcher/test/matchers_unminified_test.dart:119: 
Nit: extra newline

[blackhc, 2014-12-03 10:07:17]

Thanks for the review!

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/lib/src/core...
File pkg/matcher/lib/src/core_matchers.dart (right):

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/lib/src/core...
pkg/matcher/lib/src/core_matchers.dart:29: bool matches(item, Map matchState) {
On 2014/12/01 22:18:07, nweiz wrote:
> Nit: empty lines between members.
> 
> The existing code here is quite old and doesn't always follow the style guide,
> but new additions should do so.

Done.

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/test/matcher...
File pkg/matcher/test/matchers_unminified_test.dart (right):

https://codereview.chromium.org/763783003/diff/20001/pkg/matcher/test/matcher...
pkg/matcher/test/matchers_unminified_test.dart:119: 
On 2014/12/01 22:18:07, nweiz wrote:
> Nit: extra newline

Done.

[blackhc, 2014-12-04 10:24:40]

The CQ bit was checked by blackhc@google.com

[commit-bot: I haz the power, 2014-12-04 10:27:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-04 10:27:08]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[nweiz, 2014-12-04 21:28:13]

Committed patchset #3 (id:40001) manually as 42119 (presubmit successful).