Introduce span with line context

Introduce span with line context

R=nweiz@google.com

Committed: https://github.com/dart-lang/source_span/commit/9075bbd2d972b6ffebd4a8bebfc31cde4911add3

[Siggi Cherem (dart-lang), 2015-03-21 00:15:16]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-03-21 00:19:59]

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

[Siggi Cherem (dart-lang), 2015-03-21 00:19:59]

For background: I'd like to serialize spans that contain the full line
information without having to copy/paste all the logic in FileSpan.

https://codereview.chromium.org/1028813002/diff/20001/lib/src/file.dart
File lib/src/file.dart (left):

https://codereview.chromium.org/1028813002/diff/20001/lib/src/file.dart#oldco...
lib/src/file.dart:265: String message(String message, {color}) {
the old logic basically is now shared with the default `message` implementation

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span_mixin.dart...
lib/src/span_mixin.dart:62: var textLine;
this condition is the only new part, the rest of the code is basically what we
had in file_span.message

https://codereview.chromium.org/1028813002/diff/20001/test/file_message_test....
File test/file_message_test.dart (left):

https://codereview.chromium.org/1028813002/diff/20001/test/file_message_test....
test/file_message_test.dart:67: ^"""));
would you consider this a breaking change? 

I could preserve the old behavior, but it is nice that if there is no context
(as in the case of an empty file), there is little value for showing this empty
line after all.

[nweiz, 2015-03-24 23:01:18]

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/file.dart#newco...
lib/src/file.dart:266: String get contextLine {
Nit: put getters above the constructor.

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:81: /// A class that describes a segment of source text.
Update this doc comment.

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:84: /// Line containing the source-span
"The text of the line containing this span.

If the span covers multiple lines, this should only contain the first."

I've considered for a while having multi-line spans produce multi-line messages,
so maybe this variable's name should be updated to be forwards-compatible with
those semantics. Maybe just [context]?

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:95: this.contextLine) : super(start, end, text) {
Nit: put the super call on its own line.

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:99: 'contain "$text" starting at column ${start.column +
1}.');
I'd throw separate errors for [contextLine] not containing [text] and for the
column being wrong, so that it's easier for users to figure out what's going
wrong.

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span_context.da...
lib/src/span_context.dart:8: abstract class SourceSpanContext {
Why is this necessary? Can't FileSpan just implement SourceSpanWithContext?

https://codereview.chromium.org/1028813002/diff/20001/test/file_message_test....
File test/file_message_test.dart (left):

https://codereview.chromium.org/1028813002/diff/20001/test/file_message_test....
test/file_message_test.dart:67: ^"""));
On 2015/03/21 00:19:59, Siggi Cherem (dart-lang) wrote:
> would you consider this a breaking change? 
> 
> I could preserve the old behavior, but it is nice that if there is no context
> (as in the case of an empty file), there is little value for showing this
empty
> line after all.

No, I don't consider the specific format of [message] to be a guaranteed part of
the API.

[Siggi Cherem (dart-lang), 2015-03-25 00:33:25]

thanks, PTAL

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/file.dart#newco...
lib/src/file.dart:266: String get contextLine {
On 2015/03/24 23:01:17, nweiz wrote:
> Nit: put getters above the constructor.

Done.

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:81: /// A class that describes a segment of source text.
On 2015/03/24 23:01:17, nweiz wrote:
> Update this doc comment.

Done.

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:84: /// Line containing the source-span
On 2015/03/24 23:01:17, nweiz wrote:
> "The text of the line containing this span.
> 
> If the span covers multiple lines, this should only contain the first."
> 
> I've considered for a while having multi-line spans produce multi-line
messages,
> so maybe this variable's name should be updated to be forwards-compatible with
> those semantics. Maybe just [context]?

Sounds good. Done

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:95: this.contextLine) : super(start, end, text) {
On 2015/03/24 23:01:17, nweiz wrote:
> Nit: put the super call on its own line.

Done. This made me realized I was not running the formatter, so I went ahead and
ran it on the entire package

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:99: 'contain "$text" starting at column ${start.column +
1}.');
On 2015/03/24 23:01:17, nweiz wrote:
> I'd throw separate errors for [contextLine] not containing [text] and for the
> column being wrong, so that it's easier for users to figure out what's going
> wrong.

Done.

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span_context.da...
lib/src/span_context.dart:8: abstract class SourceSpanContext {
On 2015/03/24 23:01:17, nweiz wrote:
> Why is this necessary? Can't FileSpan just implement SourceSpanWithContext?

Done.

https://codereview.chromium.org/1028813002/diff/20001/test/file_message_test....
File test/file_message_test.dart (left):

https://codereview.chromium.org/1028813002/diff/20001/test/file_message_test....
test/file_message_test.dart:67: ^"""));
On 2015/03/24 23:01:17, nweiz wrote:
> On 2015/03/21 00:19:59, Siggi Cherem (dart-lang) wrote:
> > would you consider this a breaking change? 
> > 
> > I could preserve the old behavior, but it is nice that if there is no
context
> > (as in the case of an empty file), there is little value for showing this
> empty
> > line after all.
> 
> No, I don't consider the specific format of [message] to be a guaranteed part
of
> the API.

Turns out the fix for this was much easier than I thought, so I decided to keep
this unchanged for the time being.

[Siggi Cherem (dart-lang), 2015-03-25 00:52:22]

Patchset #3 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-25 00:53:53]

sorry - I did one more modification: I changed "SourceSpanWithContext" to be an
interface, and made the base class "SourceSpanWithContextBase".

I did this basically to be consistent with the pattern you had before with
SourceSpan vs SourceSpanBase.

[nweiz, 2015-03-25 01:10:39]

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:95: this.contextLine) : super(start, end, text) {
On 2015/03/25 00:33:25, Siggi Cherem (dart-lang) wrote:
> On 2015/03/24 23:01:17, nweiz wrote:
> > Nit: put the super call on its own line.
> 
> Done. This made me realized I was not running the formatter, so I went ahead
and
> ran it on the entire package

I actually prefer to have format-everything CLs separate from functional CLs,
since they create a lot of churn. Also, it looks like a lot of the formatting
changes for this package are the same or worse than the original (especially in
the tests), so I'd rather not include them.

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

https://codereview.chromium.org/1028813002/diff/80001/lib/src/span.dart#newco...
lib/src/span.dart:105: /// If the span covers multiple lines, this should only
contain the first.
I just realized that this property is incorrect; the constructor will throw an
error if it's true, since [context] won't contain [text] anymore. Probably this
should actually contain *all* lines the span covers; this means
[FileSpan.context]'s behavior will have to be changed.

[nweiz, 2015-03-25 01:15:20]

On 2015/03/25 00:53:53, Siggi Cherem (dart-lang) wrote:
> sorry - I did one more modification: I changed "SourceSpanWithContext" to be
an
> interface, and made the base class "SourceSpanWithContextBase".
> 
> I did this basically to be consistent with the pattern you had before with
> SourceSpan vs SourceSpanBase.

This is necessary for SourceSpan because we can't have SourceSpan use
SourceSpanMixin *and* have SourceSpanMixin implement SourceSpan. There's no such
constraint for SourceSpanWithContext, though, so it should probably only be one
class.

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

https://codereview.chromium.org/1028813002/diff/80001/lib/src/span.dart#newco...
lib/src/span.dart:61: abstract class SourceSpanWithContext extends SourceSpan {
Now that I think about it, it would be nice to have this in a separate library.

[Siggi Cherem (dart-lang), 2015-03-26 21:05:45]

Patchset #4 (id:100001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-26 21:07:30]

PTAL

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

https://codereview.chromium.org/1028813002/diff/20001/lib/src/span.dart#newco...
lib/src/span.dart:95: this.contextLine) : super(start, end, text) {
On 2015/03/25 01:10:39, nweiz wrote:
> On 2015/03/25 00:33:25, Siggi Cherem (dart-lang) wrote:
> > On 2015/03/24 23:01:17, nweiz wrote:
> > > Nit: put the super call on its own line.
> > 
> > Done. This made me realized I was not running the formatter, so I went ahead
> and
> > ran it on the entire package
> 
> I actually prefer to have format-everything CLs separate from functional CLs,
> since they create a lot of churn. Also, it looks like a lot of the formatting
> changes for this package are the same or worse than the original (especially
in
> the tests), so I'd rather not include them.

OK reverted all other formatter changes.

In dev_compiler we are running the formatter on every CL so we don't think about
it anymroe

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

https://codereview.chromium.org/1028813002/diff/80001/lib/src/span.dart#newco...
lib/src/span.dart:61: abstract class SourceSpanWithContext extends SourceSpan {
On 2015/03/25 01:15:20, nweiz wrote:
> Now that I think about it, it would be nice to have this in a separate
library.

Done.

https://codereview.chromium.org/1028813002/diff/80001/lib/src/span.dart#newco...
lib/src/span.dart:105: /// If the span covers multiple lines, this should only
contain the first.
On 2015/03/25 01:10:39, nweiz wrote:
> I just realized that this property is incorrect; the constructor will throw an
> error if it's true, since [context] won't contain [text] anymore. Probably
this
> should actually contain *all* lines the span covers; this means
> [FileSpan.context]'s behavior will have to be changed.

I went with somewhere in between. I validate that context contains the text, and
that it begins at the appropriate column within a line. When emitting messages,
I emit preceding lines, but then I still truncate the span on the first line. We
can keep adding support for multi-line spans as a separate change

[nweiz, 2015-03-26 21:49:52]

lgtm!

[Siggi Cherem (dart-lang), 2015-03-26 21:55:33]

Committed patchset #4 (id:120001) manually as
9075bbd2d972b6ffebd4a8bebfc31cde4911add3 (presubmit successful).