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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions qlty-check/src/parser/ripgrep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, // submatch uses 1-based indexing
end_line: line_number,
end_column: submatch.end,
end_column: submatch.end + 1, // submatch uses 1-based indexing
..Default::default()
}),
}),
Expand Down Expand Up @@ -137,7 +137,7 @@ mod test {
"###;

let issues = Ripgrep::default().parse("ripgrep", input);
insta::assert_yaml_snapshot!(issues.unwrap(), @r###"
insta::assert_yaml_snapshot!(issues.unwrap(), @r"
- tool: ripgrep
ruleKey: FIXME
message: // FIXME TODO
Expand All @@ -147,9 +147,9 @@ mod test {
path: basic_e.in.rs
range:
startLine: 2
startColumn: 7
startColumn: 8
endLine: 2
endColumn: 12
endColumn: 13
- tool: ripgrep
ruleKey: TODO
message: // FIXME TODO
Expand All @@ -159,9 +159,9 @@ mod test {
path: basic_e.in.rs
range:
startLine: 2
startColumn: 13
startColumn: 14
endLine: 2
endColumn: 17
endColumn: 18
- tool: ripgrep
ruleKey: NOTE
message: // NOTE
Expand All @@ -171,9 +171,9 @@ mod test {
path: basic.in.rs
range:
startLine: 2
startColumn: 7
startColumn: 8
endLine: 2
endColumn: 11
endColumn: 12
- tool: ripgrep
ruleKey: FIXME
message: // FIXME TODO
Expand All @@ -183,9 +183,9 @@ mod test {
path: basic.in.rs
range:
startLine: 3
startColumn: 7
startColumn: 8
endLine: 3
endColumn: 12
endColumn: 13
- tool: ripgrep
ruleKey: TODO
message: // FIXME TODO
Expand All @@ -195,9 +195,9 @@ mod test {
path: basic.in.rs
range:
startLine: 3
startColumn: 13
startColumn: 14
endLine: 3
endColumn: 17
endColumn: 18
- tool: ripgrep
ruleKey: HACK
message: // HACK
Expand All @@ -207,10 +207,10 @@ mod test {
path: basic.in.rs
range:
startLine: 4
startColumn: 7
startColumn: 8
endLine: 4
endColumn: 11
"###);
endColumn: 12
");
}

#[test]
Expand All @@ -231,7 +231,7 @@ mod test {
assert!(logs_contain("Failed to parse line number"));
assert!(logs_contain("Failed to parse path"));

insta::assert_yaml_snapshot!(issues.unwrap(), @r###"
insta::assert_yaml_snapshot!(issues.unwrap(), @r"
- tool: ripgrep
ruleKey: NOTE
message: // NOTE
Expand All @@ -241,7 +241,9 @@ mod test {
path: basic.in.rs
range:
startLine: 2
startColumn: 1
endLine: 2
"###);
endColumn: 1
");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.0 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 12,
"endColumn": 13,
"endLine": 3,
"startColumn": 7,
"startColumn": 8,
"startLine": 3,
},
},
Expand All @@ -36,9 +36,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.0 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 12,
"endColumn": 13,
"endLine": 7,
"startColumn": 7,
"startColumn": 8,
"startLine": 7,
},
},
Expand All @@ -63,9 +63,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.0 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 11,
"endColumn": 12,
"endLine": 4,
"startColumn": 7,
"startColumn": 8,
"startLine": 4,
},
},
Expand All @@ -90,9 +90,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.0 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 11,
"endColumn": 12,
"endLine": 2,
"startColumn": 7,
"startColumn": 8,
"startLine": 2,
},
},
Expand All @@ -117,9 +117,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.0 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 17,
"endColumn": 18,
"endLine": 3,
"startColumn": 13,
"startColumn": 14,
"startLine": 3,
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.1 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 12,
"endColumn": 13,
"endLine": 3,
"startColumn": 7,
"startColumn": 8,
"startLine": 3,
},
},
Expand All @@ -36,9 +36,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.1 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 12,
"endColumn": 13,
"endLine": 7,
"startColumn": 7,
"startColumn": 8,
"startLine": 7,
},
},
Expand All @@ -63,9 +63,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.1 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 11,
"endColumn": 12,
"endLine": 4,
"startColumn": 7,
"startColumn": 8,
"startLine": 4,
},
},
Expand All @@ -90,9 +90,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.1 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 11,
"endColumn": 12,
"endLine": 2,
"startColumn": 7,
"startColumn": 8,
"startLine": 2,
},
},
Expand All @@ -117,9 +117,9 @@ exports[`linter=ripgrep fixture=basic version=14.1.1 1`] = `
"location": {
"path": "basic.in.rs",
"range": {
"endColumn": 17,
"endColumn": 18,
"endLine": 3,
"startColumn": 13,
"startColumn": 14,
"startLine": 3,
},
},
Expand Down
13 changes: 9 additions & 4 deletions qlty-plugins/plugins/scripts/updateLinterVersions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,17 @@ const getLintersList = async (): Promise<string[]> => {
try {
const dirContents = await fs.promises.readdir(LINTERS_PATH);
const folders = await Promise.all(
dirContents.filter(async (dirContent) => {
const linterPath = path.join(LINTERS_PATH, dirContent);
return (await fs.promises.stat(linterPath)).isDirectory();
dirContents.map((dirContent) => {
return (async () => {
const linterPath = path.join(LINTERS_PATH, dirContent);
const stat = await fs.promises.stat(linterPath);
if (stat.isDirectory()) {
return linterPath;
}
})();
}),
);
return folders;
return folders.filter((folder) => folder !== undefined);
} catch (err) {
throw new Error(`Failed to read linters directory: ${err}`);
}
Expand Down
27 changes: 19 additions & 8 deletions qlty-plugins/plugins/tests/driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,15 @@ import * as os from "os";
import path from "path";
import * as git from "simple-git";
import * as util from "util";
import { OPTIONS } from "./utils";
import { getKnownGoodVersion } from "./runLinterTest";
import { OPTIONS } from "./utils";

interface Issue {
tool: string;
ruleKey: string;
path: string;
message: string;
}

const execFilePromise = util.promisify(execFile);

Expand All @@ -31,6 +38,10 @@ export const executionEnv = (sandbox: string) => {
// This is necessary to prevent launcher collision of non-atomic operations
TMPDIR: path.resolve(sandbox, TEMP_SUBDIR),
TEMP: path.resolve(sandbox, TEMP_SUBDIR),
PATH: [
path.resolve(REPO_ROOT, "..", "..", "target", "debug"),
process.env.PATH,
].join(path.delimiter),
};
};

Expand Down Expand Up @@ -179,12 +190,12 @@ export class QltyDriver {
};

output = await this.runQltyCmd(fullArgs, { env });
} catch (error: any) {
const err: {
} catch (error) {
const err = error as {
code: number;
stdout?: string;
stderr?: string;
} = error;
};
output = { stdout: err.stdout ?? "", stderr: err.stderr ?? "" };
exitCode = err.code;
}
Expand Down Expand Up @@ -240,7 +251,7 @@ export class QltyDriver {
stdout: string;
stderr: string;
exitCode: number;
outputJson: any;
outputJson: Issue[];
}) {
return {
success: [0].includes(runResult.exitCode),
Expand All @@ -252,14 +263,14 @@ export class QltyDriver {
};
}

tryParseDeterministicResults(sandboxPath: string, outputJson: any) {
tryParseDeterministicResults(sandboxPath: string, outputJson: Issue[]) {
// return function to lazy evaluate sorting and skip if not needed
return () => {
if (!outputJson) {
return undefined;
}

outputJson.sort((a: any, b: any) => {
outputJson.sort((a, b) => {
if (a.tool < b.tool) {
return -1;
}
Expand Down Expand Up @@ -288,7 +299,7 @@ export class QltyDriver {
});

// make results deterministic by removing the sandbox path
outputJson.forEach((issue: any) => {
outputJson.forEach((issue: Issue) => {
if (issue.message.includes(sandboxPath)) {
issue.message = issue.message.replace(sandboxPath, "");
}
Expand Down
5 changes: 3 additions & 2 deletions qlty-plugins/plugins/tests/runLinterTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const getVersionsForTarget = (
.map((file) => {
const fileMatch = file.match(getSnapshotRegex(prefix));

if (fileMatch) {
if (fileMatch?.groups?.version) {
matchExists = true;
return fileMatch.groups?.version;
}
Expand Down Expand Up @@ -85,7 +85,8 @@ export const getKnownGoodVersion = (dirname: string, linterName: string) => {
);

const plugin_toml = toml.parse(plugin_file);
return plugin_toml.plugins.definitions[linterName].known_good_version;
return plugin_toml.plugins.definitions[linterName]
.known_good_version as string;
};

const getSnapshotRegex = (prefix: string) =>
Expand Down
4 changes: 2 additions & 2 deletions qlty-plugins/plugins/tests/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const OPTIONS: EnvOptions = {
),
};

const extractStructure = (obj: any): Record<string, unknown> => {
const extractStructure = (obj: object): Record<string, unknown> => {
const structure: Record<string, unknown> = {};
for (const [key, value] of Object.entries(obj)) {
if (typeof value === "object" && value !== null) {
Expand All @@ -48,7 +48,7 @@ const extractStructure = (obj: any): Record<string, unknown> => {
return structure;
};

export const serializeStructure = (obj: any) => {
export const serializeStructure = (obj: object) => {
const structure = extractStructure(obj);
return JSON.stringify(structure, undefined, 2);
};
Loading