Adapt to new location format #1153
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anderseknert
force-pushed
the
no-location-text
branch
2 times, most recently
from
f8d0407 to
9c4dd67
Compare
charlieegan3
reviewed
Oct 1, 2024
mem.pprof
Outdated
There was a problem hiding this comment.
Presume this was not meant to be committed.
There was a problem hiding this comment.
It was left to see if someone would actually scroll through :P No, thanks! Removing before merge.
charlieegan3
approved these changes
Oct 1, 2024
anderseknert
force-pushed
the
no-location-text
branch
from
9c4dd67 to
2e014ee
Compare
Wow, this took quite some time to finish! But looking at the end result, I'd say it was worth it. Having Roast represent locations as `row:col:end_row:end_col` means quite a reduction in AST size, and Regal's own JSON AST goes from 12 Mb to 10 Mb (in OPA AST that is 24 Mb for comparison). Running `regal lint bundle` is also ~100 ms faster, and improvement of about 5%. Nice! But this wasn't only about performance. Having end locations available for any node means we can now easily show e.g. violations with a precise location in LSP editors, and won't have to default to end-of-line, as we previously did for most rules. This also meant that we were able to remove all code in Regal that tried to calculate end position based on the text attribute. Nice! Signed-off-by: Anders Eknert <anders@styra.com>
anderseknert
force-pushed
the
no-location-text
branch
from
2e014ee to
227c102
Compare
srenatus
pushed a commit
to srenatus/regal
that referenced
this pull request
Oct 1, 2024
Wow, this took quite some time to finish! But looking at the end result, I'd say it was worth it. Having Roast represent locations as `row:col:end_row:end_col` means quite a reduction in AST size, and Regal's own JSON AST goes from 12 Mb to 10 Mb (in OPA AST that is 24 Mb for comparison). Running `regal lint bundle` is also ~100 ms faster, and improvement of about 5%. Nice! But this wasn't only about performance. Having end locations available for any node means we can now easily show e.g. violations with a precise location in LSP editors, and won't have to default to end-of-line, as we previously did for most rules. This also meant that we were able to remove all code in Regal that tried to calculate end position based on the text attribute. Nice! Signed-off-by: Anders Eknert <anders@styra.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wow, this took quite some time to finish! But looking at the end result, I'd say it was worth it. Having Roast represent locations as
row:col:end_row:end_colmeans quite a reduction in AST size, and Regal's own JSON AST goes from 12 Mb to 10 Mb (in OPA AST that is 24 Mb for comparison).Running
regal lint bundleis also ~100 ms faster, and improvement of about 5%. Nice!But this wasn't only about performance. Having end locations available for any node means we can now easily show e.g. violations with a precise location in LSP editors, and won't have to default to end-of-line, as we previously did for most rules. This also meant that we were able to remove all code in Regal that tried to calculate end position based on the text attribute. Nice!
NOTE: this should be merged after anderseknert/roast#11 so that we can depend on a tagged roast version.