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

Fix off-by-1 offset in ripgrep column info #1427

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Conversation

lsegal
Copy link
Member

@lsegal lsegal commented Jan 14, 2025

Ripgrep uses 0-indexed column values which is not used by other linter output

Ripgrep uses 0-indexed column values which is not used by other linter output
@lsegal lsegal self-assigned this Jan 14, 2025
@lsegal lsegal requested a review from a team January 14, 2025 22:03
Copy link
Contributor

qltysh bot commented Jan 14, 2025

The code coverage on the diff in this pull request is 100.0%

Drilldown
Path File Coverage Δ

@@ -100,9 +100,9 @@ impl Parser for Ripgrep {
path: path.text.clone(),
range: Some(Range {
start_line: line_number,
start_column: submatch.start,
start_column: submatch.start + 1,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there's a docs page which references this behavior but if not could you please add a comment just clarifying the discovery and the need for this +1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haven't really found any explicit documentation for this, it was discovered while dogfooding our tools.

@@ -100,9 +100,9 @@ impl Parser for Ripgrep {
path: path.text.clone(),
range: Some(Range {
start_line: line_number,
start_column: submatch.start,
start_column: submatch.start + 1,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there's a docs page which references this behavior but if not could you please add a comment just clarifying the discovery and the need for this +1?

@marschattha
Copy link
Member

The plugin tests might also need updates

@lsegal lsegal merged commit c628088 into main Jan 15, 2025
8 of 9 checks passed
@lsegal lsegal deleted the ls/fix-ripgrep-offset branch January 15, 2025 01:03
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.

4 participants