Correctly match and print strings with escaped values

Correctly match and print strings with escaped values

R=nweiz@google.com

Committed: https://github.com/dart-lang/matcher/commit/3a1ab8c5b45f1b955314f629097c2fa02c635f32

[kevmoo, 2015-02-04 00:59:05]

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

[nweiz, 2015-02-04 01:34:46]

Unit testing a utility function is fine, but please also include tests of the
user-facing code that uses it.

https://codereview.chromium.org/891463004/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/891463004/diff/20001/CHANGELOG.md#newcode1
CHANGELOG.md:1: ## 0.11.4+2
It looks like 0.11.4+1 was never released. Can you just include this change in
that version and release it?

https://codereview.chromium.org/891463004/diff/20001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Correctly match and print `String`s with escaped values.
"with escaped values" -> "containing characters that must be represented as
escape sequences".

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart
File lib/src/escape.dart (right):

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:7: const _escapMap = const {
"escapMap" -> "escapeMap"

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:16: final _escapeStr = "[" +
_escapMap.keys.map(_getHexLiteral).join() + "]";
Just inline this in the regexp definition.

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:20: String escape(String s) {
Put this in utils. Also, document everything in here.

https://codereview.chromium.org/891463004/diff/20001/test/escape_test.dart
File test/escape_test.dart (right):

https://codereview.chromium.org/891463004/diff/20001/test/escape_test.dart#ne...
test/escape_test.dart:9: import 'package:matcher/src/escape.dart';
All "package:" imports should be in a single block, even if they're for the
current package.

https://codereview.chromium.org/891463004/diff/20001/test/escape_test.dart#ne...
test/escape_test.dart:29: void _testEscaping(String k, String source, String
target) {
Document these functions and don't use single-letter variable names, here and
below.

[kevmoo, 2015-02-04 05:28:21]

PTAL

https://codereview.chromium.org/891463004/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/891463004/diff/20001/CHANGELOG.md#newcode1
CHANGELOG.md:1: ## 0.11.4+2
On 2015/02/04 01:34:45, nweiz wrote:
> It looks like 0.11.4+1 was never released. Can you just include this change in
> that version and release it?

Done.

https://codereview.chromium.org/891463004/diff/20001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Correctly match and print `String`s with escaped values.
On 2015/02/04 01:34:45, nweiz wrote:
> "with escaped values" -> "containing characters that must be represented as
> escape sequences".

Done.

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart
File lib/src/escape.dart (right):

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:7: const _escapMap = const {
On 2015/02/04 01:34:45, nweiz wrote:
> "escapMap" -> "escapeMap"

Done.

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:16: final _escapeStr = "[" +
_escapMap.keys.map(_getHexLiteral).join() + "]";
On 2015/02/04 01:34:45, nweiz wrote:
> Just inline this in the regexp definition.

Done.

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:20: String escape(String s) {
On 2015/02/04 01:34:45, nweiz wrote:
> Put this in utils. Also, document everything in here.

Done.

https://codereview.chromium.org/891463004/diff/20001/lib/src/escape.dart#newc...
lib/src/escape.dart:20: String escape(String s) {
On 2015/02/04 01:34:45, nweiz wrote:
> Put this in utils. Also, document everything in here.

Done.

https://codereview.chromium.org/891463004/diff/20001/test/escape_test.dart
File test/escape_test.dart (right):

https://codereview.chromium.org/891463004/diff/20001/test/escape_test.dart#ne...
test/escape_test.dart:9: import 'package:matcher/src/escape.dart';
On 2015/02/04 01:34:46, nweiz wrote:
> All "package:" imports should be in a single block, even if they're for the
> current package.

Done.

https://codereview.chromium.org/891463004/diff/20001/test/escape_test.dart#ne...
test/escape_test.dart:29: void _testEscaping(String k, String source, String
target) {
On 2015/02/04 01:34:46, nweiz wrote:
> Document these functions and don't use single-letter variable names, here and
> below.

Done.

[nweiz, 2015-02-06 00:23:14]

I still don't see tests that this escaping works properly in matchers that use
it.

https://codereview.chromium.org/891463004/diff/80001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/891463004/diff/80001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Correctly match and print `String`s with containing characters
that must be
"with containing" -> "containing"

https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart
File lib/src/util.dart (right):

https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart#newcode20
lib/src/util.dart:20: /// A [RegExp] that matches whitespace characters.
"whitespace characters that should be escaped"

https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart#newcode53
lib/src/util.dart:53: return str.replaceAllMapped(_escapeRegExp, (m) {
Don't use single-letter variables.

https://codereview.chromium.org/891463004/diff/80001/test/string_matchers_tes...
File test/string_matchers_test.dart (right):

https://codereview.chromium.org/891463004/diff/80001/test/string_matchers_tes...
test/string_matchers_test.dart:15: test('Report mismatches in whitespace and
escape sequences correctly', () {
"Report" -> "Reports"

[kevmoo, 2015-02-06 02:58:59]

The change to test/string_matchers_test.dart tests that the difference in the
two files is reported correctly by identifying the correct space.

The string generated in crazy, so I'm picking a representative subset to
confirm.

https://codereview.chromium.org/891463004/diff/80001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/891463004/diff/80001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Correctly match and print `String`s with containing characters
that must be
On 2015/02/06 00:23:13, nweiz wrote:
> "with containing" -> "containing"

Done.

https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart
File lib/src/util.dart (right):

https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart#newcode20
lib/src/util.dart:20: /// A [RegExp] that matches whitespace characters.
On 2015/02/06 00:23:13, nweiz wrote:
> "whitespace characters that should be escaped"

Done.

https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart#newcode53
lib/src/util.dart:53: return str.replaceAllMapped(_escapeRegExp, (m) {
On 2015/02/06 00:23:13, nweiz wrote:
> Don't use single-letter variables.

Done.

https://codereview.chromium.org/891463004/diff/80001/test/string_matchers_tes...
File test/string_matchers_test.dart (right):

https://codereview.chromium.org/891463004/diff/80001/test/string_matchers_tes...
test/string_matchers_test.dart:15: test('Report mismatches in whitespace and
escape sequences correctly', () {
On 2015/02/06 00:23:14, nweiz wrote:
> "Report" -> "Reports"

Done.

[kevmoo, 2015-02-09 20:19:35]

On 2015/02/06 02:58:59, kevmoo wrote:
> The change to test/string_matchers_test.dart tests that the difference in the
> two files is reported correctly by identifying the correct space.
> 
> The string generated in crazy, so I'm picking a representative subset to
> confirm.
> 
> https://codereview.chromium.org/891463004/diff/80001/CHANGELOG.md
> File CHANGELOG.md (right):
> 
> https://codereview.chromium.org/891463004/diff/80001/CHANGELOG.md#newcode3
> CHANGELOG.md:3: * Correctly match and print `String`s with containing
characters
> that must be
> On 2015/02/06 00:23:13, nweiz wrote:
> > "with containing" -> "containing"
> 
> Done.
> 
> https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart
> File lib/src/util.dart (right):
> 
>
https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart#newcode20
> lib/src/util.dart:20: /// A [RegExp] that matches whitespace characters.
> On 2015/02/06 00:23:13, nweiz wrote:
> > "whitespace characters that should be escaped"
> 
> Done.
> 
>
https://codereview.chromium.org/891463004/diff/80001/lib/src/util.dart#newcode53
> lib/src/util.dart:53: return str.replaceAllMapped(_escapeRegExp, (m) {
> On 2015/02/06 00:23:13, nweiz wrote:
> > Don't use single-letter variables.
> 
> Done.
> 
>
https://codereview.chromium.org/891463004/diff/80001/test/string_matchers_tes...
> File test/string_matchers_test.dart (right):
> 
>
https://codereview.chromium.org/891463004/diff/80001/test/string_matchers_tes...
> test/string_matchers_test.dart:15: test('Report mismatches in whitespace and
> escape sequences correctly', () {
> On 2015/02/06 00:23:14, nweiz wrote:
> > "Report" -> "Reports"
> 
> Done.

Ping?

[nweiz, 2015-02-10 20:39:23]

lgtm

[kevmoo, 2015-02-10 20:45:26]

Committed patchset #6 (id:100001) manually as
3a1ab8c5b45f1b955314f629097c2fa02c635f32 (presubmit successful).