Support multiple occurrences of text in context

Support multiple occurrences of text in context

R=nweiz@google.com

Committed: https://github.com/dart-lang/source_span/commit/5c6c13f62fc111adaace3aeb4a38853d64481d06

[Siggi Cherem (dart-lang), 2015-03-30 23:34:13]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-03-30 23:34:17]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-30 23:34:24]

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

[nweiz, 2015-03-30 23:45:52]

https://codereview.chromium.org/1041163005/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1041163005/diff/40001/CHANGELOG.md#newcode4
CHANGELOG.md:4: `text` within `context.
You forgot a "`" after "context".

https://codereview.chromium.org/1041163005/diff/40001/lib/src/span_with_conte...
File lib/src/span_with_context.dart (right):

https://codereview.chromium.org/1041163005/diff/40001/lib/src/span_with_conte...
lib/src/span_with_context.dart:41: }
Shouldn't you also do this logic when constructing the message?

[Siggi Cherem (dart-lang), 2015-03-31 01:03:59]

Patchset #2 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-31 01:05:54]

Patchset #2 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-31 01:07:50]

Thanks Natalie, PTAL

https://codereview.chromium.org/1041163005/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1041163005/diff/40001/CHANGELOG.md#newcode4
CHANGELOG.md:4: `text` within `context.
On 2015/03/30 23:45:51, nweiz wrote:
> You forgot a "`" after "context".

good catch, thanks.

https://codereview.chromium.org/1041163005/diff/40001/lib/src/span_with_conte...
File lib/src/span_with_context.dart (right):

https://codereview.chromium.org/1041163005/diff/40001/lib/src/span_with_conte...
lib/src/span_with_context.dart:41: }
On 2015/03/30 23:45:51, nweiz wrote:
> Shouldn't you also do this logic when constructing the message?

good point - fixed too.

https://codereview.chromium.org/1041163005/diff/100001/lib/src/span_mixin.dart
File lib/src/span_mixin.dart (right):

https://codereview.chromium.org/1041163005/diff/100001/lib/src/span_mixin.dar...
lib/src/span_mixin.dart:68: var lineStart = findLineStart(context, text,
column);
text.split('\n').first was necessary before when FileSpan.context only had a
prefix of the containing text. Now that we have fixed that, it's OK to just
search for the full text here

[nweiz, 2015-03-31 19:22:10]

A couple more things, then lgtm

https://codereview.chromium.org/1041163005/diff/100001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1041163005/diff/100001/lib/src/utils.dart#new...
lib/src/utils.dart:42: /// Returns the index in [context] where that line
begins, or -1 if none exists.
Nit: I really like using "null" as a signal for "not found" rather than "-1".
Granted it's inconsistent with String.indexOf, but it makes it a lot more
apparent when a caller fails to handle the case than an int.

https://codereview.chromium.org/1041163005/diff/100001/test/utils_test.dart
File test/utils_test.dart (right):

https://codereview.chromium.org/1041163005/diff/100001/test/utils_test.dart#n...
test/utils_test.dart:69: }
Can you also add a test for printing a message when the context contains
multiple instances of the span?

[Siggi Cherem (dart-lang), 2015-03-31 19:51:21]

I also added a test checking for the null result in findLineStart

https://codereview.chromium.org/1041163005/diff/100001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1041163005/diff/100001/lib/src/utils.dart#new...
lib/src/utils.dart:42: /// Returns the index in [context] where that line
begins, or -1 if none exists.
On 2015/03/31 19:22:10, nweiz wrote:
> Nit: I really like using "null" as a signal for "not found" rather than "-1".
> Granted it's inconsistent with String.indexOf, but it makes it a lot more
> apparent when a caller fails to handle the case than an int.

Done.

https://codereview.chromium.org/1041163005/diff/100001/test/utils_test.dart
File test/utils_test.dart (right):

https://codereview.chromium.org/1041163005/diff/100001/test/utils_test.dart#n...
test/utils_test.dart:69: }
On 2015/03/31 19:22:10, nweiz wrote:
> Can you also add a test for printing a message when the context contains
> multiple instances of the span?

Done.

[Siggi Cherem (dart-lang), 2015-03-31 19:54:46]

Committed patchset #3 (id:120001) manually as
5c6c13f62fc111adaace3aeb4a38853d64481d06 (presubmit successful).