Expect.equals and Expect.identical escape more strings

Expect.equals and Expect.identical escape more strings

I found it difficult to understand messages with strings that look the
same, e.g. containing zero-width character or different encodings of a
character.

BUG=
R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/85af0259cb1f8c46101b3cc048828982fe10bb41

[sra1, 2017-03-14 02:46:57]

sra@google.com changed reviewers:
+ lrn@chromium.org

[sra1, 2017-03-17 05:34:56]

ping

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart
File pkg/expect/lib/expect.dart (right):

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:65: buf.write(r"\x");
I would also be happy combining \x and \u into \u{XXX}
as in

buf.write(r"\u{");
buf.write(code.toRadixString(16));
buf.write("}");

[Lasse Reichstein Nielsen, 2017-03-17 06:38:08]

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

[Lasse Reichstein Nielsen, 2017-03-17 06:38:09]

Sorry, I forgot to actually send my comments :(

LGTM

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart
File pkg/expect/lib/expect.dart (right):

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:58: static _escapeSubstring(StringBuffer buf, String
string, int start, int end) {
If we are trying to be "complete" here, then we should probably also escape
backslashes, otherwise the string 
  "\\u{123}\u{123}"
will be escaped into 
  \u{123}\u{123}
which isn't really correct.

Or maybe we should just use JSON.encode from dart:convert, it already handles
all these things (but sadly only using \u, not \x).

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:63: buf.writeCharCode(string.codeUnitAt(i));
string.codeUnitAt(i) -> code

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:65: buf.write(r"\x");
I'd prefer not doing that. I find \x more readable, probably beacuse it's
immediately clear what range it is in. Reserving \u for non-Latin-1 characters
immediately tells you that \u{XXX} isn't something you would recognize.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:70: buf.write(hexDigits[(code >> 12) & 15]);
You technically don't need "& 15" for the top bits (but then, this is clearly
not optimized for speed, so who cares? Well, apparently I do - mostly because
it's inconsistent with the two-digit case :).

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:168: if (expected is String && actual is String) {
You might want a special case here for when the strings are *equal* but not
*identical*, otherwise the error will look odd.
Can't happen in JS, but it can on the VM.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:170: ", actual: <${_escapeString(actual)}>$msg) "
In the long run, we might want a "prettify" function that can generalize to
other types as well (and be applied recursively to collections).

[sra1, 2017-04-06 21:19:18]

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart
File pkg/expect/lib/expect.dart (right):

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:58: static _escapeSubstring(StringBuffer buf, String
string, int start, int end) {
On 2017/03/17 06:38:08, Lasse Reichstein Nielsen wrote:
> If we are trying to be "complete" here, then we should probably also escape
> backslashes, otherwise the string 
>   "\\u{123}\u{123}"
> will be escaped into 
>   \u{123}\u{123}
> which isn't really correct.
> 
> Or maybe we should just use JSON.encode from dart:convert, it already handles
> all these things (but sadly only using \u, not \x).

Done.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:63: buf.writeCharCode(string.codeUnitAt(i));
On 2017/03/17 06:38:08, Lasse Reichstein Nielsen wrote:
> string.codeUnitAt(i) -> code

Done.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:65: buf.write(r"\x");
On 2017/03/17 06:38:08, Lasse Reichstein Nielsen wrote:
> I'd prefer not doing that. I find \x more readable, probably beacuse it's
> immediately clear what range it is in. Reserving \u for non-Latin-1 characters
> immediately tells you that \u{XXX} isn't something you would recognize.

Acknowledged.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:70: buf.write(hexDigits[(code >> 12) & 15]);
On 2017/03/17 06:38:08, Lasse Reichstein Nielsen wrote:
> You technically don't need "& 15" for the top bits (but then, this is clearly
> not optimized for speed, so who cares? Well, apparently I do - mostly because
> it's inconsistent with the two-digit case :).

Acknowledged.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:168: if (expected is String && actual is String) {
On 2017/03/17 06:38:08, Lasse Reichstein Nielsen wrote:
> You might want a special case here for when the strings are *equal* but not
> *identical*, otherwise the error will look odd.
> Can't happen in JS, but it can on the VM.

Acknowledged.

https://codereview.chromium.org/2750763002/diff/1/pkg/expect/lib/expect.dart#...
pkg/expect/lib/expect.dart:170: ", actual: <${_escapeString(actual)}>$msg) "
On 2017/03/17 06:38:08, Lasse Reichstein Nielsen wrote:
> In the long run, we might want a "prettify" function that can generalize to
> other types as well (and be applied recursively to collections).

Acknowledged.

[sra1, 2017-04-06 22:39:02]

Description was changed from

==========
Expect.equals and Expect.identical escape more strings

I found it difficult to understand messages with strings that look the
same, e.g. containing zero-width character or different encodings of a
character.

BUG=
==========

to

==========
Expect.equals and Expect.identical escape more strings

I found it difficult to understand messages with strings that look the
same, e.g. containing zero-width character or different encodings of a
character.

BUG=
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85af0259cb1f8c46101b3cc048828982fe10bb41
==========

[sra1, 2017-04-06 22:39:03]

Committed patchset #2 (id:20001) manually as
85af0259cb1f8c46101b3cc048828982fe10bb41 (presubmit successful).