Skip to content
This repository was archived by the owner on Jan 14, 2025. It is now read-only.

Support the latest source_span #34

Merged
merged 2 commits into from
Feb 16, 2019
Merged

Support the latest source_span #34

merged 2 commits into from
Feb 16, 2019

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Jan 26, 2019

No description provided.

@nex3 nex3 requested a review from natebosch January 26, 2019 00:09
@@ -84,15 +84,15 @@ main() {
_span(2, 1, map, file),
"line 2, column 1: \n"
" ,\n"
"2 | 0*23456789\n"
" | ^\n"
"1 | 0*23456789\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why doesn't this match the line number above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. That looks like a bug. I'll fix it in source_span.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it looks like this is trickier than I thought. We're in a difficult position where there's no way, given a point span (that is, a span with length 0) at column 0 and a context line (such as "0*23456789\n"), to disambiguate whether that point refers to the beginning of that line or the beginning of the next line. In this case it's the former, but it may be the latter for spans at the end of files that end in newlines.

We should really just have an index field in SourceSpanWithContext to disambiguate this, but adding that is non-trivial. A number of downstream users implement FileSpan as an interface, which implements SourceSpanWithContext. This means that we can't add new fields without it being a breaking change.

In the long term, we probably want to release source_span 2.0.0 with that field, as well as probably a FileSpanMixin so that we can tell users "don't treat this as an interface". In the meantime, I'll make sure that FileSpan.context only includes the line that the span appears on. This'll make end-of-file spans look bad, but it'll fix this case, which is probably more common.

source_span 1.5.3 was generating the wrong output here.
@nex3
Copy link
Contributor Author

nex3 commented Jan 29, 2019

@natebosch I fixed source_span and updated these tests. PTAL

@nex3 nex3 merged commit cef9700 into master Feb 16, 2019
@nex3 nex3 deleted the source-span branch February 16, 2019 01:52
mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants