-
-
Notifications
You must be signed in to change notification settings - Fork 73
Linter: Use --generate-todo to create a todo list for ignoring rules. #685
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,8 @@ import { colorize } from "@herb-tools/highlighter" | |||||||||
|
|
||||||||||
| import type { Diagnostic } from "@herb-tools/core" | ||||||||||
| import type { FormatOption } from "./argument-parser.js" | ||||||||||
| import { LintOffense } from "../types.js" | ||||||||||
| import { LinterTodo } from "../linter-todo.js" | ||||||||||
|
|
||||||||||
| export interface ProcessedFile { | ||||||||||
| filename: string | ||||||||||
|
|
@@ -19,6 +21,7 @@ export interface ProcessingContext { | |||||||||
| projectPath?: string | ||||||||||
| pattern?: string | ||||||||||
| fix?: boolean | ||||||||||
| generateTodo?: boolean | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export interface ProcessingResult { | ||||||||||
|
|
@@ -35,6 +38,7 @@ export interface ProcessingResult { | |||||||||
|
|
||||||||||
| export class FileProcessor { | ||||||||||
| private linter: Linter | null = null | ||||||||||
| private linterTodo: LinterTodo | null = null | ||||||||||
|
|
||||||||||
| private isRuleAutocorrectable(ruleName: string): boolean { | ||||||||||
| if (!this.linter) return false | ||||||||||
|
|
@@ -51,6 +55,15 @@ export class FileProcessor { | |||||||||
| } | ||||||||||
|
|
||||||||||
| async processFiles(files: string[], formatOption: FormatOption = 'detailed', context?: ProcessingContext): Promise<ProcessingResult> { | ||||||||||
|
|
||||||||||
| if (context?.projectPath) { | ||||||||||
| this.linterTodo = new LinterTodo(context.projectPath) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (context?.generateTodo) { | ||||||||||
| this.linterTodo?.clearTodoFile() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| let totalErrors = 0 | ||||||||||
| let totalWarnings = 0 | ||||||||||
| let totalIgnored = 0 | ||||||||||
|
|
@@ -60,7 +73,9 @@ export class FileProcessor { | |||||||||
| const allOffenses: ProcessedFile[] = [] | ||||||||||
| const ruleOffenses = new Map<string, { count: number, files: Set<string> }>() | ||||||||||
|
|
||||||||||
| for (const filename of files) { | ||||||||||
| const todoOffenses: Record<string, LintOffense[]> = {} | ||||||||||
|
|
||||||||||
| for (const filename of files) { | ||||||||||
| const filePath = context?.projectPath ? resolve(context.projectPath, filename) : resolve(filename) | ||||||||||
| let content = readFileSync(filePath, "utf-8") | ||||||||||
| const parseResult = Herb.parse(content) | ||||||||||
|
|
@@ -85,10 +100,13 @@ export class FileProcessor { | |||||||||
| } | ||||||||||
|
|
||||||||||
| if (!this.linter) { | ||||||||||
| this.linter = new Linter(Herb) | ||||||||||
| this.linter = new Linter(Herb, undefined, this.linterTodo) | ||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can refactor the Linter to take in an
Suggested change
or just:
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| const lintResult = this.linter.lint(content, { fileName: filename }) | ||||||||||
| if (context?.generateTodo) { | ||||||||||
| todoOffenses[filename] = lintResult.offenses | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (ruleCount === 0) { | ||||||||||
| ruleCount = this.linter.getRuleCount() | ||||||||||
|
|
@@ -154,6 +172,10 @@ export class FileProcessor { | |||||||||
| totalIgnored += lintResult.ignored | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (context?.generateTodo) { | ||||||||||
| this.linterTodo?.generateTodoConfig(todoOffenses) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return { totalErrors, totalWarnings, totalIgnored, filesWithOffenses, filesFixed, ruleCount, allOffenses, ruleOffenses, context } | ||||||||||
| } | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import YAML from "yaml" | ||
| import { existsSync, unlinkSync, readFileSync, writeFileSync } from "fs" | ||
| import { join, relative, isAbsolute } from "path" | ||
|
|
||
| import { LinterRule, LintOffense } from "./types" | ||
|
|
||
| export interface TodoConfig { | ||
| excludes: { | ||
| [rule: LinterRule]: { | ||
| [filePath: string]: { | ||
| warning: number | ||
| error: number | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export class LinterTodo { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is not the best. Maybe |
||
| private static readonly TODO_FILE = ".herb-todo.yml" | ||
| private todoConfig: TodoConfig | null = null | ||
| private readonly projectPath: string | ||
| private readonly todoPath: string | ||
|
|
||
| constructor(projectPath: string) { | ||
| this.projectPath = projectPath | ||
| this.todoPath = join(this.projectPath, LinterTodo.TODO_FILE) | ||
| this.loadTodoConfig() | ||
| } | ||
|
|
||
| clearTodoFile(): void { | ||
| if (!this.todoExists()) return | ||
| unlinkSync(this.todoPath) | ||
| this.loadTodoConfig() | ||
| } | ||
|
|
||
| generateTodoConfig(offenses: Record<string, LintOffense[]>): void { | ||
| const config: TodoConfig = { excludes: {} } | ||
| for (const filePath of Object.keys(offenses)) { | ||
| const fileOffenses = offenses[filePath] | ||
| const relativePath = isAbsolute(filePath) ? relative(this.projectPath, filePath) : filePath | ||
| for (const offense of fileOffenses) { | ||
| const ruleName = offense.rule | ||
| const ruleEntry = (config.excludes[ruleName] ??= {}) | ||
| const ruleBaseline = (ruleEntry[relativePath] ??= { warning: 0, error: 0 }) | ||
|
|
||
| if (offense.severity !== "warning" && offense.severity !== "error") { | ||
| continue | ||
| } | ||
|
|
||
| ruleBaseline[offense.severity]++ | ||
| } | ||
| } | ||
| writeFileSync(this.todoPath, YAML.stringify(config)) | ||
| } | ||
|
|
||
| filterOffenses( | ||
| offenses: LintOffense[], | ||
| filePath: string, | ||
| ): LintOffense[] { | ||
| if (!this.todoConfig) return offenses | ||
|
|
||
| const relativePath = isAbsolute(filePath) ? relative(this.projectPath, filePath): filePath | ||
| const filteredOffenses: LintOffense[] = [] | ||
|
|
||
| const ruleOffensesCounts = new Map<LinterRule, { error: number; warning: number }>() | ||
|
|
||
| for (const offense of offenses) { | ||
| if (offense.severity !== "error" && offense.severity !== "warning") { | ||
| filteredOffenses.push(offense) | ||
| continue | ||
| } | ||
|
|
||
| const ruleEntry = this.todoConfig.excludes[offense.rule] | ||
| const ruleBaseline = ruleEntry ? ruleEntry[relativePath] : undefined | ||
|
|
||
| if (!ruleBaseline) { | ||
| filteredOffenses.push(offense) | ||
| continue | ||
| } | ||
|
|
||
| if (!ruleOffensesCounts.has(offense.rule)) { | ||
| ruleOffensesCounts.set(offense.rule, { error: 0, warning: 0 }) | ||
| } | ||
|
|
||
| const ruleCounts = ruleOffensesCounts.get(offense.rule)! | ||
|
|
||
| if (ruleCounts[offense.severity] < ruleBaseline[offense.severity]) { | ||
| ruleCounts[offense.severity]++ | ||
| continue | ||
| } | ||
|
|
||
| filteredOffenses.push(offense) | ||
| } | ||
|
|
||
| return filteredOffenses | ||
| } | ||
|
|
||
| private todoExists(): boolean { | ||
| return existsSync(this.todoPath) | ||
| } | ||
|
|
||
| private loadTodoConfig(): void { | ||
| if (!this.todoExists()) { | ||
| this.todoConfig = { excludes: {} } | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| const content = readFileSync(this.todoPath, "utf8") | ||
| const parsed: TodoConfig = YAML.parse(content) | ||
| this.todoConfig = parsed | ||
| } catch { | ||
| console.log( | ||
| "Warning: Failed to load .herb-todo.yml. Ignoring todo configuration.", | ||
| ) | ||
| this.todoConfig = { excludes: {} } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
--regenerate-todoas rubocop, or--generate-todoas standardrb. I think regenerate makes clear that you are destroying the old one. But generate-todo is a little less verbose and simple.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can have both so there is no surprise.
--generate-todocan generate a file if none is present yet. If one is present it could back out and say something like:.herb-todo.yml already exists, run "--regenerate-todo" to overwrite it.--regenerate-todowould just always overwrite the current one, no matter if.herb-todo.ymlexists or not.