Extract out a source_span package from source_maps.

Extract out a source_span package from source_maps.

This is just the first step; future CLs will add support for the new API to
various packages currently using the old one.

BUG=19930
R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2014-07-11 17:01:32]

Hey Nathan, would it be possible to create a diff with the old code to make this
easier to review?

For example, coping the old files in the same place as the new ones, and
uploading a patch that shows the diff between the two?

I also have some general questions, but I think it will be easier to chat in
person about them.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/CHANGELOG.md
File pkg/source_span/CHANGELOG.md (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/CHANGELOG.md...
pkg/source_span/CHANGELOG.md:31: * `SourceFileSegment` has been removed. This
class wasn't widely used and was
I don't think we should remove this entirely. This is not used today in polymer
because we don't yet have support for piping source maps through barback, but
otherwise we would be using it.

In web_ui we use it to be able to parse the .dart code in an inlined <script>
tag inside html.

[nweiz, 2014-07-11 18:41:41]

> Hey Nathan, would it be possible to create a diff with the old code to make
this easier to review?

I'll do this if you want, but line-by-line the code is very different so I doubt
it'll provide much insight beyond what you'd get from reading it anew.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/CHANGELOG.md
File pkg/source_span/CHANGELOG.md (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/CHANGELOG.md...
pkg/source_span/CHANGELOG.md:31: * `SourceFileSegment` has been removed. This
class wasn't widely used and was
On 2014/07/11 17:01:32, Siggi Cherem (dart-lang) wrote:
> I don't think we should remove this entirely. This is not used today in
polymer
> because we don't yet have support for piping source maps through barback, but
> otherwise we would be using it.
> 
> In web_ui we use it to be able to parse the .dart code in an inlined <script>
> tag inside html.

As it exists currently in source_maps, it's not a good general-purpose API. It's
extremely inconsistent about what's treated as absolute vs relative to the base;
[location] takes a relative offset, but [getLine] and [getColumn] take absolute
offsets, for example.

If that API works for polymer, then maybe that's where the class should live,
but I don't think it's widely useful enough to be in this package.

[Siggi Cherem (dart-lang), 2014-07-16 21:26:10]

Looks nice overall, just a few suggestions and questions.

One general comment on naming of files. I'm not sure there is much value in
having the 'source_' prefix on every file under 'src', given that 'source_' is
already in the package name and the main library name.

How about keeping it short and name the files 'src/file.dart', 'src/span.dart',
'src/location.dart' 'src/span_mixin.dart', 'src/span_exception.dart'?

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_file.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_file.dart:150: new
String.fromCharCodes(_decodedChars.sublist(start, end));
we use to take the max between start and 0, is your intention to change this to
throw as well? will the underlying API throw as you expect or do we want a
different RangeError here?

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_file.dart:212: var endSpan = end == this.end ?
this : other;
could we reuse more of the code from the mixin? for example, we could pass an
optional second argument with a factory and do something like:

return super.union(other, (s, e, _) => new FileSpan._(file, s, e));

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_location.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_location.dart:30: String get editorString {
rename 'editorString'? I'm thinking this is also used for other tools. Some
ideas:
 - toolsString
 - machineFormat
 - machineString

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
I don't think we should make disjointness an error.

This is not used much, but were we use it so far we assume it is ok to give
disjoint spans. There is an example usage in html5lib, where we give the first
and last of a list of spans to compute the range where all of them occur.

We could also add a separate method that checks for this if you like, for
example 'append' or 'expand' might be a reasonable name for that?

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:56: /// it indicates that the text
shouldn't be highlighted.
that's a lot of semantics in one argument. I don't know if that is a bit
confusing for people. If not, let's keep it. Otherwise, one option is to bring
back 2 variables (useColors, color), but make it implicit that if color is set,
useColors is implied to be true.

So you mostly would end up using color or useColors but not both. Thoughts?

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:65: _SourceSpan(this.start, this.end,
this.text) {
why not make this class public? alternatively, make the checks in the
constructor an extra method in the mixin, so others can use that too. For
instance,

_SourceSpan(this.start, this.end, this.text) {
  checkValidInput();
}

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span_exception.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span_exception.dart:38: : super(message, span);
there were some changes in the source_maps package on this exception recently,
would you like to copy those changes here too?

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span_mixin.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span_mixin.dart:44:
endSpan.text.substring(beginSpan.end.distance(endSpan.start));
for disjoint unions, we could use a text with ' ... ' in the middle or null
text, for union in File spans, we can use the original source file to figure out
the text.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span_mixin.dart:59: var textLine =
text.split("\n").first;
not sure I follow why just the first line?

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
File pkg/source_span/test/source_span_test.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
pkg/source_span/test/source_span_test.dart:144: group("message()", () {
Let's bring back some of the tests we have in the source_maps package. We had a
lot of subtle bugs in the past and many of those tests were there as regression
coverage. In particular: 
 - tests for getLine/getColumn for every character of the source text (we
automated that process that given the line + column + line-starts computes the
offset, then verify that calling getLine/getColumn returns the same as the
input.

 - putting the '^' correctly in the right column
 - putting the '^' and terminating the line correctly if the span is in the last
line of a source file (this was actually trick the first time we did this)

There are probably more that I can't remember, so it might be a good idea to
copy a few tests from there. Many of the tests used a file that had digits as
data. This was useful to easily validate the columns you use in the spans match,
since they would also match the numbers in the text.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
pkg/source_span/test/source_span_test.dart:146: expect(span.message("oh no"),
equalsIgnoringWhitespace("""
I'd remove the 'equalsIgnoringWhitespace and just remove a few leading
characters. In the current test the underline ^^ could be off below and we
wouldn't be able to tell.

[nweiz, 2014-07-17 20:22:09]

> How about keeping it short and name the files 'src/file.dart',
'src/span.dart',
'src/location.dart' 'src/span_mixin.dart', 'src/span_exception.dart'?

Done.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_file.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_file.dart:150: new
String.fromCharCodes(_decodedChars.sublist(start, end));
On 2014/07/16 21:26:09, Siggi Cherem (dart-lang) wrote:
> we use to take the max between start and 0, is your intention to change this
to
> throw as well? will the underlying API throw as you expect or do we want a
> different RangeError here?

Sublist will throw appropriate errors here. This is tested in source_file_test.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_file.dart:212: var endSpan = end == this.end ?
this : other;
On 2014/07/16 21:26:09, Siggi Cherem (dart-lang) wrote:
> could we reuse more of the code from the mixin? for example, we could pass an
> optional second argument with a factory and do something like:
> 
> return super.union(other, (s, e, _) => new FileSpan._(file, s, e));

I think we really want to avoid accessing the spans' text properties if
possible, since they're O(file size) to compute. I don't think these four lines
are too much duplication.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_location.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_location.dart:30: String get editorString {
On 2014/07/16 21:26:09, Siggi Cherem (dart-lang) wrote:
> rename 'editorString'? I'm thinking this is also used for other tools. Some
> ideas:
>  - toolsString
>  - machineFormat
>  - machineString

Good point, done.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> I don't think we should make disjointness an error.
> 
> This is not used much, but were we use it so far we assume it is ok to give
> disjoint spans. There is an example usage in html5lib, where we give the first
> and last of a list of spans to compute the range where all of them occur.
> 
> We could also add a separate method that checks for this if you like, for
> example 'append' or 'expand' might be a reasonable name for that? 

I made it forbid disjoint spans because I really wanted it to work with any sort
of span, and in order to compute the text of the union of a non-FileSpan, the
full text being spanned has to be available.

This restriction doesn't necessarily apply to FileSpans, though, so I'll add an
expand method for those in particular.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:56: /// it indicates that the text
shouldn't be highlighted.
On 2014/07/16 21:26:09, Siggi Cherem (dart-lang) wrote:
> that's a lot of semantics in one argument. I don't know if that is a bit
> confusing for people. If not, let's keep it. Otherwise, one option is to bring
> back 2 variables (useColors, color), but make it implicit that if color is
set,
> useColors is implied to be true.
> 
> So you mostly would end up using color or useColors but not both. Thoughts?

I agree that this is packing "color" a little full, but I think it reads really
well in either use:

    print(span.message("oh no", color: true));
    print(span.message("oh no", color: colors.RED));

I also like that it makes it easier to implement methods that forward arguments
to this (e.g. SpanException.toString) and that it uses the short name "color"
for the most common use-case of enabling colorization without choosing a color.
I think users will understand pretty well what this parameter does once they use
it once or twice.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:65: _SourceSpan(this.start, this.end,
this.text) {
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> why not make this class public?

I tried to put this stuff in SourceSpan, but it can't include SourceSpanMixin
and also be implemented by SourceSpanMixin. I can make this public as
SourceSpanBase, though.

> alternatively, make the checks in the
> constructor an extra method in the mixin, so others can use that too. For
> instance,
> 
> _SourceSpan(this.start, this.end, this.text) {
>   checkValidInput();
> }

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span_exception.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span_exception.dart:38: : super(message, span);
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> there were some changes in the source_maps package on this exception recently,
> would you like to copy those changes here too?

Done.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span_mixin.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span_mixin.dart:44:
endSpan.text.substring(beginSpan.end.distance(endSpan.start));
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> for disjoint unions, we could use a text with ' ... ' in the middle or null
> text, for union in File spans, we can use the original source file to figure
out
> the text.

I don't like the idea of making [text] not match [length], or even of having it
not be actual text from the document. Users should be confident that the spans
returned by these methods are just as accurate as the spans they put in.

For an alternate possibility, though, see FileSpan.expand.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span_mixin.dart:59: var textLine =
text.split("\n").first;
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> not sure I follow why just the first line?

To match the old behavior. I'd like to look into making this work better for
multiline spans, but that's something for another CL.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
File pkg/source_span/test/source_span_test.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
pkg/source_span/test/source_span_test.dart:144: group("message()", () {
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> Let's bring back some of the tests we have in the source_maps package. We had
a
> lot of subtle bugs in the past and many of those tests were there as
regression
> coverage. In particular: 
>  - tests for getLine/getColumn for every character of the source text (we
> automated that process that given the line + column + line-starts computes the
> offset, then verify that calling getLine/getColumn returns the same as the
> input.

I'm not a big fan of automating test generation like this. I find it obfuscates
the specific cases that are likely to be problematic; I prefer calling out those
cases explicitly in test descriptions like I do here. If there are specific
cases beyond those I have there that you think should be tested, I'll add
individual tests for them, but I'd rather not add a test for every character in
a file.

>  - putting the '^' correctly in the right column
>  - putting the '^' and terminating the line correctly if the span is in the
last
> line of a source file (this was actually trick the first time we did this)

There are tests for both of these in file_message_test.

> There are probably more that I can't remember, so it might be a good idea to
> copy a few tests from there. Many of the tests used a file that had digits as
> data. This was useful to easily validate the columns you use in the spans
match,
> since they would also match the numbers in the text.

I think I tested all the tricky cases, but if I missed one let me know.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
pkg/source_span/test/source_span_test.dart:146: expect(span.message("oh no"),
equalsIgnoringWhitespace("""
On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> I'd remove the 'equalsIgnoringWhitespace and just remove a few leading
> characters. In the current test the underline ^^ could be off below and we
> wouldn't be able to tell.

Done.

[Siggi Cherem (dart-lang), 2014-07-17 22:24:40]

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
On 2014/07/17 20:22:08, nweiz wrote:
> On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > I don't think we should make disjointness an error.
> > 
> > This is not used much, but were we use it so far we assume it is ok to give
> > disjoint spans. There is an example usage in html5lib, where we give the
first
> > and last of a list of spans to compute the range where all of them occur.
> > 
> > We could also add a separate method that checks for this if you like, for
> > example 'append' or 'expand' might be a reasonable name for that? 
> 
> I made it forbid disjoint spans because I really wanted it to work with any
sort
> of span, and in order to compute the text of the union of a non-FileSpan, the
> full text being spanned has to be available.
> 
> This restriction doesn't necessarily apply to FileSpans, though, so I'll add
an
> expand method for those in particular.

Ah, I had not realized until now that you made `text` mandatory. It used to be
optional. I actually think we still support making it optional.

If text is optional - then we can make it `null` for disjoint unions. At that
point instead of `expand`, we could add an optional argument here:

  SourceSpan union(SourceSpan other, {allowDisjoint: false})

So:
- if they are disjoint & allowDisjoint == false, we get the current behavior
- if they are disjoint and `allowDisjoint: true`, then `text` may be null if it
can't be computed (e.g. it wont be null for FileSpan)

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:56: /// it indicates that the text
shouldn't be highlighted.
On 2014/07/17 20:22:08, nweiz wrote:
> On 2014/07/16 21:26:09, Siggi Cherem (dart-lang) wrote:
> > that's a lot of semantics in one argument. I don't know if that is a bit
> > confusing for people. If not, let's keep it. Otherwise, one option is to
bring
> > back 2 variables (useColors, color), but make it implicit that if color is
> set,
> > useColors is implied to be true.
> > 
> > So you mostly would end up using color or useColors but not both. Thoughts?
> 
> I agree that this is packing "color" a little full, but I think it reads
really
> well in either use:
> 
>     print(span.message("oh no", color: true));
>     print(span.message("oh no", color: colors.RED));
> 
> I also like that it makes it easier to implement methods that forward
arguments
> to this (e.g. SpanException.toString) and that it uses the short name "color"
> for the most common use-case of enabling colorization without choosing a
color.
> I think users will understand pretty well what this parameter does once they
use
> it once or twice.

ok. sounds good

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:65: _SourceSpan(this.start, this.end,
this.text) {
On 2014/07/17 20:22:08, nweiz wrote:
> On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > why not make this class public?
> 
> I tried to put this stuff in SourceSpan, but it can't include SourceSpanMixin
> and also be implemented by SourceSpanMixin. I can make this public as
> SourceSpanBase, though.
> 
> > alternatively, make the checks in the
> > constructor an extra method in the mixin, so others can use that too. For
> > instance,
> > 
> > _SourceSpan(this.start, this.end, this.text) {
> >   checkValidInput();
> > }
> 

Your idea sounds good, it seems to match what we do for List/ListBase/ListMixin.
The alternative would be to declare all the abstract members in the mixin
instead (remove the implements from there). Them people can directly extend from
SourceSpan.

I'm ok with either approach. Up to you.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
File pkg/source_span/test/source_span_test.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
pkg/source_span/test/source_span_test.dart:144: group("message()", () {
On 2014/07/17 20:22:08, nweiz wrote:
> On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > Let's bring back some of the tests we have in the source_maps package. We
had
> a
> > lot of subtle bugs in the past and many of those tests were there as
> regression
> > coverage. In particular: 
> >  - tests for getLine/getColumn for every character of the source text (we
> > automated that process that given the line + column + line-starts computes
the
> > offset, then verify that calling getLine/getColumn returns the same as the
> > input.
> 
> I'm not a big fan of automating test generation like this. I find it
obfuscates
> the specific cases that are likely to be problematic; I prefer calling out
those
> cases explicitly in test descriptions like I do here. If there are specific
> cases beyond those I have there that you think should be tested, I'll add
> individual tests for them, but I'd rather not add a test for every character
in
> a file.
> 
> >  - putting the '^' correctly in the right column
> >  - putting the '^' and terminating the line correctly if the span is in the
> last
> > line of a source file (this was actually trick the first time we did this)
> 
> There are tests for both of these in file_message_test.
> 
> > There are probably more that I can't remember, so it might be a good idea to
> > copy a few tests from there. Many of the tests used a file that had digits
as
> > data. This was useful to easily validate the columns you use in the spans
> match,
> > since they would also match the numbers in the text.
> 
> I think I tested all the tricky cases, but if I missed one let me know.

ok - one that just crossed my mind is to test `file.message` for a file that has
a single line without an EOL. This might be similar to your test with the empty
file + location point, but for a span.

https://codereview.chromium.org/381363002/diff/20001/pkg/source_span/test/spa...
File pkg/source_span/test/span_test.dart (right):

https://codereview.chromium.org/381363002/diff/20001/pkg/source_span/test/spa...
pkg/source_span/test/span_test.dart:156: expect(span.message("oh no"),
equalsIgnoringWhitespace("""
you might have missed this one

[nweiz, 2014-07-17 23:18:15]

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
On 2014/07/17 22:24:39, Siggi Cherem (dart-lang) wrote:
> On 2014/07/17 20:22:08, nweiz wrote:
> > On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > > I don't think we should make disjointness an error.
> > > 
> > > This is not used much, but were we use it so far we assume it is ok to
give
> > > disjoint spans. There is an example usage in html5lib, where we give the
> first
> > > and last of a list of spans to compute the range where all of them occur.
> > > 
> > > We could also add a separate method that checks for this if you like, for
> > > example 'append' or 'expand' might be a reasonable name for that? 
> > 
> > I made it forbid disjoint spans because I really wanted it to work with any
> sort
> > of span, and in order to compute the text of the union of a non-FileSpan,
the
> > full text being spanned has to be available.
> > 
> > This restriction doesn't necessarily apply to FileSpans, though, so I'll add
> an
> > expand method for those in particular.
> 
> Ah, I had not realized until now that you made `text` mandatory. It used to be
> optional. I actually think we still support making it optional.
> 
> If text is optional - then we can make it `null` for disjoint unions. At that
> point instead of `expand`, we could add an optional argument here:
> 
>   SourceSpan union(SourceSpan other, {allowDisjoint: false})
> 
> So:
> - if they are disjoint & allowDisjoint == false, we get the current behavior
> - if they are disjoint and `allowDisjoint: true`, then `text` may be null if
it
> can't be computed (e.g. it wont be null for FileSpan)

Do we have a use case for spans without text available? My intuition says that
if a span is being created, the creator knows the text it's associated with.

The reason I'm pushing back on this so hard is that I'm very sympathetic to
people writing code to process arbitrary spans. Every additional variation on
valid formats for the fields a span exposes makes their lives harder, since they
have to gracefully handle all those variations. So if we can restrict the set of
valid spans without hurting span generators—which I think we can here—I think
it's worth doing so.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:65: _SourceSpan(this.start, this.end,
this.text) {
On 2014/07/17 22:24:39, Siggi Cherem (dart-lang) wrote:
> On 2014/07/17 20:22:08, nweiz wrote:
> > On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > > why not make this class public?
> > 
> > I tried to put this stuff in SourceSpan, but it can't include
SourceSpanMixin
> > and also be implemented by SourceSpanMixin. I can make this public as
> > SourceSpanBase, though.
> > 
> > > alternatively, make the checks in the
> > > constructor an extra method in the mixin, so others can use that too. For
> > > instance,
> > > 
> > > _SourceSpan(this.start, this.end, this.text) {
> > >   checkValidInput();
> > > }
> > 
> 
> Your idea sounds good, it seems to match what we do for
List/ListBase/ListMixin.
> The alternative would be to declare all the abstract members in the mixin
> instead (remove the implements from there). Them people can directly extend
from
> SourceSpan.

I like this idea a lot, but it means that someone writing just "extends
SourceSpanMixin" or "with SourceSpanMixin" won't get a class that inherits from
SourceSpan, which I think is too confusing.

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
File pkg/source_span/test/source_span_test.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/test/source_...
pkg/source_span/test/source_span_test.dart:144: group("message()", () {
On 2014/07/17 22:24:39, Siggi Cherem (dart-lang) wrote:
> On 2014/07/17 20:22:08, nweiz wrote:
> > On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > > Let's bring back some of the tests we have in the source_maps package. We
> had
> > a
> > > lot of subtle bugs in the past and many of those tests were there as
> > regression
> > > coverage. In particular: 
> > >  - tests for getLine/getColumn for every character of the source text (we
> > > automated that process that given the line + column + line-starts computes
> the
> > > offset, then verify that calling getLine/getColumn returns the same as the
> > > input.
> > 
> > I'm not a big fan of automating test generation like this. I find it
> obfuscates
> > the specific cases that are likely to be problematic; I prefer calling out
> those
> > cases explicitly in test descriptions like I do here. If there are specific
> > cases beyond those I have there that you think should be tested, I'll add
> > individual tests for them, but I'd rather not add a test for every character
> in
> > a file.
> > 
> > >  - putting the '^' correctly in the right column
> > >  - putting the '^' and terminating the line correctly if the span is in
the
> > last
> > > line of a source file (this was actually trick the first time we did this)
> > 
> > There are tests for both of these in file_message_test.
> > 
> > > There are probably more that I can't remember, so it might be a good idea
to
> > > copy a few tests from there. Many of the tests used a file that had digits
> as
> > > data. This was useful to easily validate the columns you use in the spans
> > match,
> > > since they would also match the numbers in the text.
> > 
> > I think I tested all the tricky cases, but if I missed one let me know.
> 
> ok - one that just crossed my mind is to test `file.message` for a file that
has
> a single line without an EOL. This might be similar to your test with the
empty
> file + location point, but for a span.

Done.

[Siggi Cherem (dart-lang), 2014-07-18 00:11:54]

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
On 2014/07/17 23:18:15, nweiz wrote:
> On 2014/07/17 22:24:39, Siggi Cherem (dart-lang) wrote:
> > On 2014/07/17 20:22:08, nweiz wrote:
> > > On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > > > I don't think we should make disjointness an error.
> > > > 
> > > > This is not used much, but were we use it so far we assume it is ok to
> give
> > > > disjoint spans. There is an example usage in html5lib, where we give the
> > first
> > > > and last of a list of spans to compute the range where all of them
occur.
> > > > 
> > > > We could also add a separate method that checks for this if you like,
for
> > > > example 'append' or 'expand' might be a reasonable name for that? 
> > > 
> > > I made it forbid disjoint spans because I really wanted it to work with
any
> > sort
> > > of span, and in order to compute the text of the union of a non-FileSpan,
> the
> > > full text being spanned has to be available.
> > > 
> > > This restriction doesn't necessarily apply to FileSpans, though, so I'll
add
> > an
> > > expand method for those in particular.
> > 
> > Ah, I had not realized until now that you made `text` mandatory. It used to
be
> > optional. I actually think we still support making it optional.
> > 
> > If text is optional - then we can make it `null` for disjoint unions. At
that
> > point instead of `expand`, we could add an optional argument here:
> > 
> >   SourceSpan union(SourceSpan other, {allowDisjoint: false})
> > 
> > So:
> > - if they are disjoint & allowDisjoint == false, we get the current behavior
> > - if they are disjoint and `allowDisjoint: true`, then `text` may be null if
> it
> > can't be computed (e.g. it wont be null for FileSpan)
> 
> Do we have a use case for spans without text available? My intuition says that
> if a span is being created, the creator knows the text it's associated with.
> 
> The reason I'm pushing back on this so hard is that I'm very sympathetic to
> people writing code to process arbitrary spans. Every additional variation on
> valid formats for the fields a span exposes makes their lives harder, since
they
> have to gracefully handle all those variations. So if we can restrict the set
of
> valid spans without hurting span generators—which I think we can here—I think
> it's worth doing so.

I understand.

The only use-case I've seen is in the source_maps package itself. The parser
produces spans when reading data out of the source map file. When a token is an
identifier, it returns a span with text, when it is not, it returns a span with
no text. In the latter case, however, it seems that we are returning a
single-point span, so we can actually legitimately set the text to be empty.

We can always add it later I guess - and I think if we were to add it, we could
do it as a non-breaking change.

For now - I'd like to either include allowDisjoint for FileSpan or expose
'expand' so we can easily migrate html5lib to use the new SourceSpans in the
near future.

As for names for that method, what are your thoughts on which should be which?
- `expand` and `append` sound stricter to me, as if they are a better fit for
the non-disjoint model.
- `union` could work for both, but when I think of Span as ranges, I think of
`union` as allowing disjointness.
- I'm trying to think of other names that sound more permissive, but I haven't
come up with something that convinces me (`enclose`, `cover`)?

[nweiz, 2014-07-18 00:30:24]

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
On 2014/07/18 00:11:54, Siggi Cherem (dart-lang) wrote:
> On 2014/07/17 23:18:15, nweiz wrote:
> > On 2014/07/17 22:24:39, Siggi Cherem (dart-lang) wrote:
> > > On 2014/07/17 20:22:08, nweiz wrote:
> > > > On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > > > > I don't think we should make disjointness an error.
> > > > > 
> > > > > This is not used much, but were we use it so far we assume it is ok to
> > give
> > > > > disjoint spans. There is an example usage in html5lib, where we give
the
> > > first
> > > > > and last of a list of spans to compute the range where all of them
> occur.
> > > > > 
> > > > > We could also add a separate method that checks for this if you like,
> for
> > > > > example 'append' or 'expand' might be a reasonable name for that? 
> > > > 
> > > > I made it forbid disjoint spans because I really wanted it to work with
> any
> > > sort
> > > > of span, and in order to compute the text of the union of a
non-FileSpan,
> > the
> > > > full text being spanned has to be available.
> > > > 
> > > > This restriction doesn't necessarily apply to FileSpans, though, so I'll
> add
> > > an
> > > > expand method for those in particular.
> > > 
> > > Ah, I had not realized until now that you made `text` mandatory. It used
to
> be
> > > optional. I actually think we still support making it optional.
> > > 
> > > If text is optional - then we can make it `null` for disjoint unions. At
> that
> > > point instead of `expand`, we could add an optional argument here:
> > > 
> > >   SourceSpan union(SourceSpan other, {allowDisjoint: false})
> > > 
> > > So:
> > > - if they are disjoint & allowDisjoint == false, we get the current
behavior
> > > - if they are disjoint and `allowDisjoint: true`, then `text` may be null
if
> > it
> > > can't be computed (e.g. it wont be null for FileSpan)
> > 
> > Do we have a use case for spans without text available? My intuition says
that
> > if a span is being created, the creator knows the text it's associated with.
> > 
> > The reason I'm pushing back on this so hard is that I'm very sympathetic to
> > people writing code to process arbitrary spans. Every additional variation
on
> > valid formats for the fields a span exposes makes their lives harder, since
> they
> > have to gracefully handle all those variations. So if we can restrict the
set
> of
> > valid spans without hurting span generators—which I think we can here—I
think
> > it's worth doing so.
> 
> I understand.
> 
> The only use-case I've seen is in the source_maps package itself. The parser
> produces spans when reading data out of the source map file. When a token is
an
> identifier, it returns a span with text, when it is not, it returns a span
with
> no text. In the latter case, however, it seems that we are returning a
> single-point span, so we can actually legitimately set the text to be empty.
> 
> We can always add it later I guess - and I think if we were to add it, we
could
> do it as a non-breaking change.
> 
> For now - I'd like to either include allowDisjoint for FileSpan or expose
> 'expand' so we can easily migrate html5lib to use the new SourceSpans in the
> near future.
> 
> As for names for that method, what are your thoughts on which should be which?
> - `expand` and `append` sound stricter to me, as if they are a better fit for
> the non-disjoint model.
> - `union` could work for both, but when I think of Span as ranges, I think of
> `union` as allowing disjointness.
> - I'm trying to think of other names that sound more permissive, but I haven't
> come up with something that convinces me (`enclose`, `cover`)?

I like "expand"—I've added it to FileSpan, which I think is sufficient to cover
html5lib's use case.

[Siggi Cherem (dart-lang), 2014-07-18 00:45:24]

lgtm! Thanks!

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
File pkg/source_span/lib/src/source_span.dart (right):

https://codereview.chromium.org/381363002/diff/1/pkg/source_span/lib/src/sour...
pkg/source_span/lib/src/source_span.dart:40: /// The two spans must have the
same source URL and may not be disjoint.
On 2014/07/18 00:30:24, nweiz wrote:
> On 2014/07/18 00:11:54, Siggi Cherem (dart-lang) wrote:
> > On 2014/07/17 23:18:15, nweiz wrote:
> > > On 2014/07/17 22:24:39, Siggi Cherem (dart-lang) wrote:
> > > > On 2014/07/17 20:22:08, nweiz wrote:
> > > > > On 2014/07/16 21:26:10, Siggi Cherem (dart-lang) wrote:
> > > > > > I don't think we should make disjointness an error.
> > > > > > 
> > > > > > This is not used much, but were we use it so far we assume it is ok
to
> > > give
> > > > > > disjoint spans. There is an example usage in html5lib, where we give
> the
> > > > first
> > > > > > and last of a list of spans to compute the range where all of them
> > occur.
> > > > > > 
> > > > > > We could also add a separate method that checks for this if you
like,
> > for
> > > > > > example 'append' or 'expand' might be a reasonable name for that? 
> > > > > 
> > > > > I made it forbid disjoint spans because I really wanted it to work
with
> > any
> > > > sort
> > > > > of span, and in order to compute the text of the union of a
> non-FileSpan,
> > > the
> > > > > full text being spanned has to be available.
> > > > > 
> > > > > This restriction doesn't necessarily apply to FileSpans, though, so
I'll
> > add
> > > > an
> > > > > expand method for those in particular.
> > > > 
> > > > Ah, I had not realized until now that you made `text` mandatory. It used
> to
> > be
> > > > optional. I actually think we still support making it optional.
> > > > 
> > > > If text is optional - then we can make it `null` for disjoint unions. At
> > that
> > > > point instead of `expand`, we could add an optional argument here:
> > > > 
> > > >   SourceSpan union(SourceSpan other, {allowDisjoint: false})
> > > > 
> > > > So:
> > > > - if they are disjoint & allowDisjoint == false, we get the current
> behavior
> > > > - if they are disjoint and `allowDisjoint: true`, then `text` may be
null
> if
> > > it
> > > > can't be computed (e.g. it wont be null for FileSpan)
> > > 
> > > Do we have a use case for spans without text available? My intuition says
> that
> > > if a span is being created, the creator knows the text it's associated
with.
> > > 
> > > The reason I'm pushing back on this so hard is that I'm very sympathetic
to
> > > people writing code to process arbitrary spans. Every additional variation
> on
> > > valid formats for the fields a span exposes makes their lives harder,
since
> > they
> > > have to gracefully handle all those variations. So if we can restrict the
> set
> > of
> > > valid spans without hurting span generators—which I think we can here—I
> think
> > > it's worth doing so.
> > 
> > I understand.
> > 
> > The only use-case I've seen is in the source_maps package itself. The parser
> > produces spans when reading data out of the source map file. When a token is
> an
> > identifier, it returns a span with text, when it is not, it returns a span
> with
> > no text. In the latter case, however, it seems that we are returning a
> > single-point span, so we can actually legitimately set the text to be empty.
> > 
> > We can always add it later I guess - and I think if we were to add it, we
> could
> > do it as a non-breaking change.
> > 
> > For now - I'd like to either include allowDisjoint for FileSpan or expose
> > 'expand' so we can easily migrate html5lib to use the new SourceSpans in the
> > near future.
> > 
> > As for names for that method, what are your thoughts on which should be
which?
> > - `expand` and `append` sound stricter to me, as if they are a better fit
for
> > the non-disjoint model.
> > - `union` could work for both, but when I think of Span as ranges, I think
of
> > `union` as allowing disjointness.
> > - I'm trying to think of other names that sound more permissive, but I
haven't
> > come up with something that convinces me (`enclose`, `cover`)?
> 
> I like "expand"—I've added it to FileSpan, which I think is sufficient to
cover
> html5lib's use case.

Ah - thanks, I somehow missed it. After the file rename there were no diffs to
guide me :(

[nweiz, 2014-07-18 00:51:49]

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