mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 14:16:28 +03:00
* Add inline, in-file ignore comments for the detector (issue #283) Complement config ignores with eslint-disable-style waivers that live where they apply and travel with the file when it leaves the repo. The motivating case is a generated/exported standalone document that legitimately uses a first-party brand typeface (on the overused-font list) and is later scanned without .impeccable/config.json present. Marker is comment-syntax-agnostic (works in //, /* */, <!-- -->, #, {/* */}): impeccable-disable <rule>[, <rule>...] [-- reason | : reason] whole file impeccable-disable-line <rule>... same line impeccable-disable-next-line <rule>... next line Bare directive or * means every rule; reason is optional and discarded at scan time. Behavior is suppression, for parity with config ignores. Implementation: - New pure module cli/engine/shared/inline-ignores.mjs (parser + filter, no Node deps). Static-HTML findings have no line number, so only whole-file directives apply there -- exactly the standalone-document case; the regex/text engine additionally honors the line-scoped forms. - Wired into detectText and detectHtml, gated by options.inlineIgnores. - detect CLI applies inline ignores by default; --no-inline-ignores skips just them, --no-config skips config and inline ignores together. Docs: config.md (new section), detector.md, README. skill/reference/hooks.md reversed its prior "inline comments are not supported" guidance and now points the agent to inline waivers for the travels-with-the-file case. Changelog 3.x. Tests: tests/inline-ignores.test.mjs (parser units, detectText/detectHtml integration, CLI end-to-end), registered in the detector suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Reconcile design hook wording with inline ignores Two hook-side fixes prompted by review of the new inline-ignore feature: 1. Clean-ack steer line. The old line ("Keep typography hierarchy, spacing rhythm, and color contrast intentional on the next change.") read as an odd non-sequitur after "No anti-patterns." Reworded the whole clean ack to say what it means: a clean scan only clears the deterministic rule set, not overall design quality, so keep following the design system and skill guidance. Now: "Design hook scanned X. No deterministic design-quality issues found. That does not mean the design is good: keep following the project design system and the impeccable skill guidance." 2. Directive footer. It still told the agent "Do not add source comments such as `impeccable: ignore`; those pollute the code and do not suppress hook findings." That is now misleading: the hook runs the same detector engine as the CLI, which honors inline `impeccable-disable` waivers, so they DO suppress hook findings (consistent with config ignores, which filterFindings already honors). Reworded to: don't silence a real finding to skip fixing it; suppress only after the user confirms intent; prefer a config ignore, and reach for an inline `impeccable-disable <rule>` comment only when the waiver must travel with a file that leaves the repo. Added a hook test asserting an inline `impeccable-disable-line` comment makes the hook scan the file clean (locks in the cross-cutting behavior), and updated the clean-ack / footer assertions to the new wording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review on inline-ignores parser - Case-insensitive fast-path bail-out (Cursor): the cheap substring guard was lowercase-only while DIRECTIVE_RE has the `i` flag, so a mixed-case marker like `Impeccable-Disable` skipped parsing entirely and never suppressed. Switched the guard to `/impeccable-disable/i.test(...)`. Added a regression test. - Removed the unreachable `-->` branch from TRAILING_CLOSER_RE (Greptile): `--+>` already matches `-->` and any longer dash run. - Replaced the always-truthy lazy-match + `if (sep)` reason strip with an explicit first-separator slice (Greptile): clearer and drops the dead branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Align inline-ignore line numbering with the detector (CRLF/CR endings) parseInlineIgnores split lines with /\r\n|\r|\n/, but detectText numbers lines with split('\n'). On classic `\r`-only endings the two diverged, so a disable-line / disable-next-line directive could key a different line than the finding it should waive (Cursor review). Split on '\n' only, matching the detector exactly; the directive regex already excludes '\r', so a trailing '\r' on CRLF files is never captured into the rule list. Added a CRLF regression test through the real detectText. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
211 lines
9.3 KiB
JavaScript
211 lines
9.3 KiB
JavaScript
import { describe, test, expect } from 'bun:test';
|
|
import fs from 'node:fs';
|
|
import os from 'node:os';
|
|
import path from 'node:path';
|
|
import { spawnSync } from 'node:child_process';
|
|
|
|
import {
|
|
parseInlineIgnores,
|
|
applyInlineIgnores,
|
|
isInlineIgnored,
|
|
} from '../cli/engine/shared/inline-ignores.mjs';
|
|
import { detectText, detectHtml } from '../cli/engine/detect-antipatterns.mjs';
|
|
|
|
const CLI = path.resolve('cli/bin/cli.js');
|
|
|
|
function rules(finding) {
|
|
return finding.antipattern;
|
|
}
|
|
|
|
describe('parseInlineIgnores', () => {
|
|
test('whole-file directive collects rules', () => {
|
|
const d = parseInlineIgnores('/* impeccable-disable overused-font, bounce-easing */');
|
|
expect([...d.file].sort()).toEqual(['bounce-easing', 'overused-font']);
|
|
expect(d.line.size).toBe(0);
|
|
expect(d.nextLine.size).toBe(0);
|
|
});
|
|
|
|
test('bare directive and explicit * both mean every rule', () => {
|
|
expect([...parseInlineIgnores('// impeccable-disable').file]).toEqual(['*']);
|
|
expect([...parseInlineIgnores('// impeccable-disable *').file]).toEqual(['*']);
|
|
});
|
|
|
|
test('disable-line targets its own line, disable-next-line targets the line below', () => {
|
|
const content = [
|
|
'a', // line 1
|
|
'b /* impeccable-disable-line overused-font */', // line 2
|
|
'// impeccable-disable-next-line side-tab', // line 3 -> targets line 4
|
|
'd', // line 4
|
|
].join('\n');
|
|
const d = parseInlineIgnores(content);
|
|
expect([...d.line.get(2)]).toEqual(['overused-font']);
|
|
expect([...d.nextLine.get(4)]).toEqual(['side-tab']);
|
|
});
|
|
|
|
test('strips eslint -- and biome : reasons from the rule list', () => {
|
|
expect([...parseInlineIgnores('// impeccable-disable overused-font -- brand font, exported doc').file])
|
|
.toEqual(['overused-font']);
|
|
expect([...parseInlineIgnores('# impeccable-disable bounce-easing: intentional bounce').file])
|
|
.toEqual(['bounce-easing']);
|
|
});
|
|
|
|
test('strips trailing comment closers across syntaxes', () => {
|
|
expect([...parseInlineIgnores('<!-- impeccable-disable overused-font -->').file]).toEqual(['overused-font']);
|
|
expect([...parseInlineIgnores('{/* impeccable-disable overused-font */}').file]).toEqual(['overused-font']);
|
|
expect([...parseInlineIgnores('{# impeccable-disable overused-font #}').file]).toEqual(['overused-font']);
|
|
});
|
|
|
|
test('directive keyword is case-insensitive (fast-path matches the regex)', () => {
|
|
expect([...parseInlineIgnores('// Impeccable-Disable overused-font').file]).toEqual(['overused-font']);
|
|
expect([...parseInlineIgnores('/* IMPECCABLE-DISABLE-LINE side-tab */').line.get(1)]).toEqual(['side-tab']);
|
|
});
|
|
|
|
test('no directive present is a cheap no-op', () => {
|
|
const d = parseInlineIgnores('.a { color: red }');
|
|
expect(d.file.size).toBe(0);
|
|
expect(d.line.size).toBe(0);
|
|
expect(d.nextLine.size).toBe(0);
|
|
});
|
|
});
|
|
|
|
describe('applyInlineIgnores / isInlineIgnored', () => {
|
|
const findings = [
|
|
{ antipattern: 'overused-font', line: 5 },
|
|
{ antipattern: 'side-tab', line: 5 },
|
|
{ antipattern: 'overused-font', line: 0 }, // no line (static-HTML shape)
|
|
];
|
|
|
|
test('whole-file directive drops every matching finding regardless of line', () => {
|
|
const out = applyInlineIgnores(findings, '/* impeccable-disable overused-font */');
|
|
expect(out.map(rules)).toEqual(['side-tab']);
|
|
});
|
|
|
|
test('* drops everything', () => {
|
|
expect(applyInlineIgnores(findings, '// impeccable-disable *')).toEqual([]);
|
|
});
|
|
|
|
test('line-scoped directive only affects the matching line and rule', () => {
|
|
const content = ['', '', '', '', 'x /* impeccable-disable-line overused-font */'].join('\n');
|
|
const out = applyInlineIgnores(findings, content);
|
|
// the line-5 overused-font goes; side-tab on line 5 and the line-less one stay
|
|
expect(out.map(rules).sort()).toEqual(['overused-font', 'side-tab']);
|
|
expect(out.some((f) => f.antipattern === 'overused-font' && f.line === 5)).toBe(false);
|
|
});
|
|
|
|
test('returns the input untouched when there are no directives', () => {
|
|
const out = applyInlineIgnores(findings, '.a {}');
|
|
expect(out).toBe(findings);
|
|
});
|
|
|
|
test('isInlineIgnored never matches a line-scoped directive for a line-less finding', () => {
|
|
const d = parseInlineIgnores('x /* impeccable-disable-line overused-font */');
|
|
expect(isInlineIgnored({ antipattern: 'overused-font', line: 0 }, d)).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe('detectText honors inline directives', () => {
|
|
const opts = { providers: [] };
|
|
|
|
test('disable-line suppresses a same-line finding', () => {
|
|
const flagged = detectText('.a { font-family: Inter; }', 'a.css', opts);
|
|
expect(flagged.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
|
|
const waived = detectText('.a { font-family: Inter; } /* impeccable-disable-line overused-font */', 'a.css', opts);
|
|
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
|
});
|
|
|
|
test('disable-next-line suppresses the finding on the following line', () => {
|
|
const content = '/* impeccable-disable-next-line overused-font */\n.a { font-family: Inter; }';
|
|
const waived = detectText(content, 'a.css', opts);
|
|
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
|
});
|
|
|
|
test('whole-file directive suppresses regardless of where the finding is', () => {
|
|
const content = '/* impeccable-disable overused-font */\n.a {}\n.b { font-family: Inter; }';
|
|
const waived = detectText(content, 'a.css', opts);
|
|
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
|
});
|
|
|
|
test('inlineIgnores:false bypasses the directive', () => {
|
|
const content = '.a { font-family: Inter; } /* impeccable-disable-line overused-font */';
|
|
const raw = detectText(content, 'a.css', { providers: [], inlineIgnores: false });
|
|
expect(raw.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
});
|
|
|
|
test('line keys align with detector line numbers on CRLF endings', () => {
|
|
// detectText numbers lines with split('\n'); parseInlineIgnores must match.
|
|
const content = '.a { font-family: Inter; }\r\n.b { font-family: Roboto; } /* impeccable-disable-line overused-font */';
|
|
const out = detectText(content, 'a.css', opts);
|
|
const fonts = out.filter((f) => f.antipattern === 'overused-font').map((f) => f.line);
|
|
expect(fonts).toEqual([1]); // Inter on line 1 stays; Roboto on line 2 is waived
|
|
});
|
|
|
|
test('a directive for one rule leaves other findings intact', () => {
|
|
const content = '.a { font-family: Inter; } /* impeccable-disable-line side-tab */';
|
|
const out = detectText(content, 'a.css', opts);
|
|
expect(out.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
});
|
|
});
|
|
|
|
describe('detectHtml honors whole-file directives (line-less findings)', () => {
|
|
const page = (extra = '') => `<!DOCTYPE html><html><head>${extra}
|
|
<style>body { font-family: Inter, sans-serif; }</style></head>
|
|
<body><p>Some real paragraph text here for the typography pass.</p>
|
|
<h1>Heading</h1><h2>Sub</h2></body></html>`;
|
|
|
|
test('overused-font fires without a directive', async () => {
|
|
const flagged = await detectHtml(await writeTmp(page()), { providers: [] });
|
|
expect(flagged.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
});
|
|
|
|
test('whole-file directive in an HTML comment suppresses it', async () => {
|
|
const file = await writeTmp(page('<!-- impeccable-disable overused-font -- exported brand doc -->'));
|
|
const waived = await detectHtml(file, { providers: [] });
|
|
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
|
});
|
|
|
|
test('inlineIgnores:false bypasses it', async () => {
|
|
const file = await writeTmp(page('<!-- impeccable-disable overused-font -->'));
|
|
const raw = await detectHtml(file, { providers: [], inlineIgnores: false });
|
|
expect(raw.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
});
|
|
});
|
|
|
|
describe('detect CLI end-to-end', () => {
|
|
function run(args) {
|
|
return spawnSync(process.execPath, [CLI, 'detect', ...args], { encoding: 'utf-8' });
|
|
}
|
|
|
|
test('inline directive is honored by default, --no-inline-ignores and --no-config bypass it', async () => {
|
|
const file = await writeTmp(
|
|
'<!DOCTYPE html><html><head><!-- impeccable-disable overused-font -->\n' +
|
|
'<style>body { font-family: Inter, sans-serif; }</style></head>\n' +
|
|
'<body><p>Paragraph copy for the typography analyzer to read.</p><h1>H</h1><h2>S</h2></body></html>',
|
|
'.html',
|
|
);
|
|
|
|
const honored = run([file, '--json', '--no-design-system']);
|
|
expect(JSON.parse(honored.stdout).some((f) => f.antipattern === 'overused-font')).toBe(false);
|
|
|
|
const bypassed = run([file, '--json', '--no-design-system', '--no-inline-ignores']);
|
|
expect(JSON.parse(bypassed.stdout).some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
|
|
const rawConfig = run([file, '--json', '--no-config']);
|
|
expect(JSON.parse(rawConfig.stdout).some((f) => f.antipattern === 'overused-font')).toBe(true);
|
|
});
|
|
});
|
|
|
|
let tmpDir;
|
|
async function writeTmp(content, ext = '.html') {
|
|
if (!tmpDir) tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-inline-'));
|
|
const file = path.join(tmpDir, `f${Math.abs(hash(content))}${ext}`);
|
|
fs.writeFileSync(file, content);
|
|
return file;
|
|
}
|
|
|
|
function hash(str) {
|
|
let h = 0;
|
|
for (let i = 0; i < str.length; i++) h = (Math.imul(31, h) + str.charCodeAt(i)) | 0;
|
|
return h;
|
|
}
|