Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Begin to include end location where applicable #917

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

anderseknert
Copy link
Member

This PR attempts to establish end location as an optional attribute of location data returned in violations, and updates ~10 rules in the bugs category to use this convention.

If no end location is provided, the default is as it used to be — end location will be set to the end of the line. For some rules, this is the right thing to do anyway, and therefore, not all rules will need to be updated..

That obviously doesn't "fix" #654, but it's a start, and as far as I've seen so far, works quite well. Importantly this change is non-breaking method for API users! Ideally, we'd put the current "row" and "col" into a "start" object too, but that would be a breaking change, and one that would impact all API users, so let's consider that for our first stable release instead.

NOTE that this does not change anything in the output of regal lint using the default "pretty"-reporter, but is only included in the more detailed JSON and SARIF formats. First and foremost this change helps LSP clients display diagnostics more accurately, and that is really where this is most useful.

When this has been merged, I'll create some new issues to get this backported to each of the rule categories.

This PR attempts to establish end location as an optional attribute of location
data returned in violations, and updates ~10 rules in the bugs category to use
this convention.

If no end location is provided, the default is as it used to be — end location
will be set to the end of the line. For some rules, this is the right thing to
do anyway, and therefore, not all rules will need to be updated..

That obviously doesn't "fix" #654, but it's a start, and as far as I've seen so
far, works quite well. Importantly this change is non-breaking method for API
users! Ideally, we'd put the current "row" and "col" into a "start" object too,
but that *would* be a breaking change, and one that would impact all API users,
so let's consider that for our first stable release instead.

NOTE that this does not change anything in the output of `regal lint` using the
default "pretty"-reporter, but is only included in the more detailed JSON and
SARIF formats. First and foremost this change helps LSP clients display
diagnostics more accurately, and that is really where this is most useful.

Signed-off-by: Anders Eknert <[email protected]>
@anderseknert anderseknert changed the title Begin to Include end location where applicable Begin to include end location where applicable Jul 15, 2024
Copy link
Member

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for working on this while you're out. I'll get it in, do some testing and start on the release.

@charlieegan3 charlieegan3 merged commit 9ed8ec1 into main Jul 16, 2024
3 checks passed
@charlieegan3 charlieegan3 deleted the end-location branch July 16, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants