Add LineScanner and SpanScanner classes to string_scanner.

Add LineScanner and SpanScanner classes to string_scanner.

R=rnystrom@google.com

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

[Bob Nystrom, 2014-05-28 21:28:57]

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/CHANG...
File pkg/string_scanner/CHANGELOG.md (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/CHANG...
pkg/string_scanner/CHANGELOG.md:3: * `new StringScanner` now takes an optional
`sourceUrl` argument that provides
Nit, but a ctor call without () looks weird to me.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/CHANG...
pkg/string_scanner/CHANGELOG.md:6: * Add `StringScanner.readChar` and
`StringScanner.peekChar` methods for doing
And here too, since otherwise a reader might think these are getters.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/exception.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/exception.dart:15: final String string;
Given that you use "sourceUrl" below, how about "source" for this?

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/exception.dart:32: String toString({bool useColors:
false, String color}) {
I'd personally prefer not using a default value here. It likely won't do what
the user expects if they pass in null for useColors. (That may happen if they
are forwarding a default value from their method. For example, if you didn't
want to provide a default value here and instead just forward to
.getLocationMessage() to "inherit" its default value, it won't actually do the
right thing. The null will override the default value.)

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/line_scanner.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/line_scanner.dart:11: /// The scanner's current
(zero-based) line number.
Almost every other line/column reporter I've seen (or written) has been
one-based. One-based offends me morally, but it seems to be the standard here.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/line_scanner.dart:31: }
Nice.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/line_scanner.dart:82: _line = newlines.length;
+=?

Does this take into account newlines after the current position but before the
matched text?

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/string_scanner.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/string_scanner.dart:80: if (index >= string.length ||
index < 0) return null;
Tiny nit, but I think it reads a bit better if the < 0 branch is first.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/utils.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/utils.dart:24: }
Check for position + length going past the end?

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
File pkg/string_scanner/test/error_test.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
pkg/string_scanner/test/error_test.dart:103: test('supports an empty length', ()
{
"empty" -> "zero".

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
File pkg/string_scanner/test/line_scanner_test.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
pkg/string_scanner/test/line_scanner_test.dart:81: test("backard through no
newlines sets the column", () {
"backward"

[nweiz, 2014-05-28 23:56:34]

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/CHANG...
File pkg/string_scanner/CHANGELOG.md (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/CHANG...
pkg/string_scanner/CHANGELOG.md:3: * `new StringScanner` now takes an optional
`sourceUrl` argument that provides
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> Nit, but a ctor call without () looks weird to me.

Done.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/CHANG...
pkg/string_scanner/CHANGELOG.md:6: * Add `StringScanner.readChar` and
`StringScanner.peekChar` methods for doing
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> And here too, since otherwise a reader might think these are getters.

Done.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/exception.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/exception.dart:15: final String string;
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> Given that you use "sourceUrl" below, how about "source" for this?

I think it's better that it match [StringScanner.string].

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/exception.dart:32: String toString({bool useColors:
false, String color}) {
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> I'd personally prefer not using a default value here. It likely won't do what
> the user expects if they pass in null for useColors. (That may happen if they
> are forwarding a default value from their method. For example, if you didn't
> want to provide a default value here and instead just forward to
> .getLocationMessage() to "inherit" its default value, it won't actually do the
> right thing. The null will override the default value.)

I generally agree for non-boolean values, but for booleans I think there are
good reasons to include the default. The user needs to understand what the
default value is regardless of whether it's declared explicitly, much more than
for non-boolean values, since they're directly deciding its value whether or not
they pass in a value. Calls to methods with boolean parameters are never
resilient to changes in the default value.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/line_scanner.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/line_scanner.dart:11: /// The scanner's current
(zero-based) line number.
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> Almost every other line/column reporter I've seen (or written) has been
> one-based. One-based offends me morally, but it seems to be the standard here.

I want to match the source_maps package here, which uses zero-based numbering.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/line_scanner.dart:82: _line = newlines.length;
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> +=?

Done.

> Does this take into account newlines after the current position but before the
> matched text?

lastMatch[0] is the entire text from the previous position to the current one.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/string_scanner.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/string_scanner.dart:80: if (index >= string.length ||
index < 0) return null;
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> Tiny nit, but I think it reads a bit better if the < 0 branch is first.

Done.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/utils.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/utils.dart:24: }
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> Check for position + length going past the end?

Done.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
File pkg/string_scanner/test/error_test.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
pkg/string_scanner/test/error_test.dart:103: test('supports an empty length', ()
{
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> "empty" -> "zero".

Done.

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
File pkg/string_scanner/test/line_scanner_test.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/test/...
pkg/string_scanner/test/line_scanner_test.dart:81: test("backard through no
newlines sets the column", () {
On 2014/05/28 21:28:57, Bob Nystrom wrote:
> "backward"

Done.

[Bob Nystrom, 2014-05-29 15:50:59]

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/exception.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/exception.dart:32: String toString({bool useColors:
false, String color}) {
On 2014/05/28 23:56:34, nweiz wrote:
> On 2014/05/28 21:28:57, Bob Nystrom wrote:
> > I'd personally prefer not using a default value here. It likely won't do
what
> > the user expects if they pass in null for useColors. (That may happen if
they
> > are forwarding a default value from their method. For example, if you didn't
> > want to provide a default value here and instead just forward to
> > .getLocationMessage() to "inherit" its default value, it won't actually do
the
> > right thing. The null will override the default value.)
> 
> I generally agree for non-boolean values, but for booleans I think there are
> good reasons to include the default. The user needs to understand what the
> default value is regardless of whether it's declared explicitly, much more
than
> for non-boolean values, since they're directly deciding its value whether or
not
> they pass in a value.

I definitely agree that users need to understand what the default value is. This
is true for other primitive types too where the user doesn't expect them to be
nullable. But they could get that understanding through documentation.

I don't understand what "since they're directly deciding its value whether or
not they pass in a value." means. My concern is that I think:

someMethod();
someMethod(anArg: null);

should do the same thing. With default values, they don't.

> Calls to methods with boolean parameters are never
> resilient to changes in the default value.

I'm less concerned about the default changing (which is definitely a breaking
change in a public API) as I am about having a single point of truth and making
forwarding work transparently.

[nweiz, 2014-05-29 20:06:26]

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
File pkg/string_scanner/lib/src/exception.dart (right):

https://codereview.chromium.org/299973002/diff/20001/pkg/string_scanner/lib/s...
pkg/string_scanner/lib/src/exception.dart:32: String toString({bool useColors:
false, String color}) {
On 2014/05/29 15:51:00, Bob Nystrom wrote:
> On 2014/05/28 23:56:34, nweiz wrote:
> > On 2014/05/28 21:28:57, Bob Nystrom wrote:
> > > I'd personally prefer not using a default value here. It likely won't do
> what
> > > the user expects if they pass in null for useColors. (That may happen if
> they
> > > are forwarding a default value from their method. For example, if you
didn't
> > > want to provide a default value here and instead just forward to
> > > .getLocationMessage() to "inherit" its default value, it won't actually do
> the
> > > right thing. The null will override the default value.)
> > 
> > I generally agree for non-boolean values, but for booleans I think there are
> > good reasons to include the default. The user needs to understand what the
> > default value is regardless of whether it's declared explicitly, much more
> than
> > for non-boolean values, since they're directly deciding its value whether or
> not
> > they pass in a value.
> 
> I definitely agree that users need to understand what the default value is.
This
> is true for other primitive types too where the user doesn't expect them to be
> nullable. But they could get that understanding through documentation.
> 
> I don't understand what "since they're directly deciding its value whether or
> not they pass in a value." means. My concern is that I think:
> 
> someMethod();
> someMethod(anArg: null);
> 
> should do the same thing. With default values, they don't.
> 
> > Calls to methods with boolean parameters are never
> > resilient to changes in the default value.
> 
> I'm less concerned about the default changing (which is definitely a breaking
> change in a public API) as I am about having a single point of truth and
making
> forwarding work transparently.

I don't think allowing users to pass null to any optional argument is an
important invariant. The invariant I'm concerned about is that the function's
internal logic doesn't need to be duplicated by the caller.

I think of functions with optional (or named) arguments as analogous to
overloads in other languages: foo(mandatoryArg) has one API, and
foo(manadatoryArg, optionalArg) has a similar but distinct API. From the
perspective of the caller, the default value of optionalArg is whatever it needs
to be to make the latter API work like the former. The specific value that makes
this true shouldn't be encoded in the caller's code.

Optional booleans don't work exactly the same way, though. They're more
analogous to fooWithTrue(mandatoryArg) and fooWithFalse(mandatoryArg) functions.
When calling a method with an optional boolean, the caller always has to decide
whether they want that boolean to be true or false; they effectively have to
decide whether they're calling fooWithTrue or fooWithFalse.

What I'm trying to say is that the logic of whether the default value is "true"
or "false" isn't internal to the function. It's something the caller has to be
aware of, and as such I think it's fine to duplicate it.

[Bob Nystrom, 2014-05-29 23:55:00]

OK, LGTM!

[nweiz, 2014-05-30 00:52:48]

Committed patchset #3 manually as r36815 (presubmit successful).