xsbti.Position: add startOffset and endOffset #173
Conversation
smarter
force-pushed
the
position-range
branch
from
2fd6398 to
79ff000
Compare
smarter
force-pushed
the
position-range
branch
from
79ff000 to
1b2f2cd
Compare
|
One remaining question is whether we should also add startLine/startColumn, endLine/endColumn. If the BSP stays line-column based instead of offset based that would be needed. |
|
The validator has checked the following projects against Scala 2.12,
✅ The result is: SUCCESS |
There was a problem hiding this comment.
Thanks for doing this! @smarter
I think startOffset/endOffset is enough (usually you will give this information to an IDE that already knows how to do the conversion from offset to (line, column). In error messages, showing the positions as we do now is good enough.
I disagree. If I'm a build tool implementing the BSP, there's no IDE involved, I'm just trying to translate what Zinc gives me to something that works with the BSP, but right now the BSP defines (like the LSP): @JsonCodec final case class Position(
line: Int,
character: Int
)
@JsonCodec final case class Range(
start: Position,
end: Position
)This could be changed in the BSP but it would be inconsistent with the LSP which is probably not going to change: microsoft/language-server-protocol#96 In conclusion things are already complicated enough, let's not make them more complicated for no reason: adding a few fields to Position has an insignificant cost and it means no one has to struggle doing conversions. |
|
So then let’s add them to this PR too to avoid the extra read that would require in the bsp servers to convert offsets to line and column. |
|
Will do! |
|
Done, I'll update the zinc PR once this is merged. |
A position now has a start, an end, and a point (the existing `offset`), just like it does in the Scala compiler. This information is especially useful for displaying squiggly lines in an IDE. This commit and the next one are required for sbt/zinc#571
Positions in the Language Server Protocol and Build Server Protocol are line/column-based instead of offset-based, so this is more convenient. Computing the line/column from the offset is possible but requires reading the source file.
smarter
force-pushed
the
position-range
branch
from
cd33aad to
5e3a102
Compare
|
OK with merging? Or should I drop the 1.2.1 upgrade here too, and should I move to the 1.x branch here too? |
|
I'm ok with targeting 1.2.1 here, but we'll need a release to make the other PR work. 😄 |
|
Ping @eed3si9n, can I merge this and request a release? |
It turns out that there is more boilerplate to fill that I missed. Also add deprecation notices.
After sbt/util#173, Position now has 13 fields so TextAnalysisFormat in Zinc needs asProduct13. While we're at it we bump the max to 21 to give us some legroom.
Follow-up to the fields added in #173, and add Problem#rendered
sbt/util#173 added the ability to carry range position. This exposes it to the sbt server.
sbt/util#173 added the ability to carry range position. This exposes it to the sbt server.
Follow up to sbt/util#173 Rewritten from sbt/zinc@ed1b515
A position now has a start, an end, and a point (the existing
offset),just like it does in the Scala compiler. This information is especially
useful for displaying squiggly lines in an IDE.