feat(kvb-code-clean): lint every comment a change adds, Jinja templates included
A Jinja {# ... #} comment in a .j2 template reached a commit because no
check listed it. scripts/comments.py reports every comment and docstring
the change adds: Python through tokenize and ast, every other language
through the masked comment spans defaults.py already knows, plus Ruby,
PHP, CSS, HTML, XML and Lua. Tool directives (noqa, shellcheck, ...) are
marked so only real comments fail the run.
Step 1 of the skill now runs it. The block-comment deletion procedure
moved to comments.md next to SKILL.md to keep SKILL.md under its
600-word cap.
🎯 Fix: 92 %
💥 Break: 3 %
💸 Cost: 100 %
⏱️ Time: –
🛡️ Security: 100 %
🏆 Score: 89
🚦 Gate: ✅ 90/5, cost and time <= 100, security 100 %
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
db3c5264f2
commit
9d1aea6f1d
4 files changed
+345
-8
No files matched your search
@@ -28,15 +28,16 @@ Delete every comment the change adds except three kinds:
|
||||
Restated code, step narration, banners, "Note that", rationale prose and
|
||||
whatever the code, a name or the commit message already carries go. When in
|
||||
doubt, delete. Docstrings and script headers count: prose outside the
|
||||
parameter doc goes too, and a `#` or `//` grep misses it.
|
||||
parameter doc goes too.
|
||||
|
||||
Block-comment languages are not line-oriented: in Jinja (`{# … #}`), C-like
|
||||
(`/* … */`), HTML (`<!-- … -->`) and kin, a closer alone on its line looks
|
||||
like a comment line; deleted with the rest, the opener swallows every live
|
||||
statement down to the next closer. Delete such a block by hand, count the
|
||||
delimiters (`grep -c '{#'` against `grep -c '#}'`), re-parse the file and
|
||||
diff it against `git show HEAD:<path>`: only comment lines may leave. A green
|
||||
lint proves nothing here.
|
||||
Run `${CLAUDE_SKILL_DIR}/scripts/comments.py --repo <dir>` (`--staged`,
|
||||
`--file` as in Step 2): it lists every comment and docstring the change adds,
|
||||
Jinja `{# … #}` in `.j2` templates included; exit 1 means hits. Judge each
|
||||
hit not marked `directive` by the three kinds above, and run it again until
|
||||
only kept hits remain. A repository's own comment lint runs too, because a
|
||||
`--no-verify` commit skips the hook that would run it. Before deleting a
|
||||
block comment (`{# … #}`, `/* … */`, `<!-- … -->`), read `comments.md` next
|
||||
to this file.
|
||||
|
||||
## Step 2: defaults
|
||||
|
||||
|
||||
@@ -0,0 +1,9 @@
|
||||
# Deleting block comments
|
||||
|
||||
Block-comment languages are not line-oriented: in Jinja (`{# … #}`), C-like
|
||||
(`/* … */`), HTML (`<!-- … -->`) and kin, a closer alone on its line looks
|
||||
like a comment line; deleted with the rest, the opener swallows every live
|
||||
statement down to the next closer. Delete such a block by hand, count the
|
||||
delimiters (`grep -c '{#'` against `grep -c '#}'`), re-parse the file and
|
||||
diff it against `git show HEAD:<path>`: only comment lines may leave. A green
|
||||
lint proves nothing here.
|
||||
Executable
+199
@@ -0,0 +1,199 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Report every comment and docstring a change adds, for the agent to delete or keep.
|
||||
|
||||
Args:
|
||||
--repo: a directory inside the git work tree to scan.
|
||||
--staged: scan only the staged changes (git diff --cached) against the
|
||||
index version of each file. Without it: every change against HEAD,
|
||||
staged and unstaged, against the working tree file.
|
||||
--file: scan this whole file, every line counted as added; repeatable,
|
||||
relative to --repo or absolute. Untracked files are in no diff.
|
||||
--json: print the hits as a JSON list instead of tab-separated lines.
|
||||
|
||||
Output:
|
||||
One line per hit: file:line, language, kind (comment or docstring), the
|
||||
text, and `directive` for a tool directive (noqa, nocheck, shellcheck, ...).
|
||||
|
||||
Exit codes:
|
||||
0 no hits but directives, 1 hits, 2 usage error, git failure, or a file
|
||||
that cannot be read or, for Python, parsed.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import ast
|
||||
import bisect
|
||||
import io
|
||||
import json
|
||||
import re
|
||||
import sys
|
||||
import tokenize
|
||||
from dataclasses import asdict, dataclass
|
||||
from pathlib import Path, PurePosixPath
|
||||
|
||||
import defaults
|
||||
|
||||
EXTRA = {
|
||||
".py": ("python", defaults.DQ + "|" + defaults.SQ, defaults.HASH),
|
||||
".rb": ("ruby", defaults.DQ + "|" + defaults.SQ, defaults.HASH),
|
||||
".php": ("php", defaults.DQ + "|" + defaults.SQ, defaults.HASH + "|" + defaults.SLASH),
|
||||
".css": ("css", defaults.DQ + "|" + defaults.SQ, r"/\*.*?\*/"),
|
||||
".scss": ("css", defaults.DQ + "|" + defaults.SQ, defaults.SLASH),
|
||||
".html": ("html", "", r"<!--.*?-->"),
|
||||
".xml": ("html", "", r"<!--.*?-->"),
|
||||
".lua": ("lua", defaults.DQ + "|" + defaults.SQ, r"--\[\[.*?\]\]|--[^\n]*"),
|
||||
}
|
||||
DIRECTIVE = re.compile(
|
||||
r"\b(?:noqa|nocheck|nosec|type:\s*ignore|pragma|pylint|mypy|ruff|pyright|isort|fmt:\s*(?:on|off)"
|
||||
r"|coding[:=]|shellcheck|yamllint|hadolint|ansible-lint|markdownlint|eslint|stylelint|prettier"
|
||||
r"|rubocop|frozen_string_literal|phpcs|phpstan|psalm|jscpd|checkov|tflint)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class Hit:
|
||||
file: str
|
||||
line: int
|
||||
language: str
|
||||
kind: str
|
||||
text: str
|
||||
directive: bool
|
||||
|
||||
|
||||
def patterns(path: str) -> list[tuple[str, str, str]]:
|
||||
found = [(lang, *defaults.LANGS[lang][:2]) for lang in defaults.languages(path) if lang != "python"]
|
||||
stem = defaults.JINJA_SUFFIX.sub("", PurePosixPath(path).name)
|
||||
extra = EXTRA.get(PurePosixPath(stem).suffix)
|
||||
return found + ([extra] if extra else [])
|
||||
|
||||
|
||||
def python_lang(path: str) -> bool:
|
||||
return defaults.languages(path) == ["python"]
|
||||
|
||||
|
||||
def known(path: str) -> bool:
|
||||
return python_lang(path) or bool(patterns(path))
|
||||
|
||||
|
||||
def touched(lines: set[int], wanted: set[int] | None) -> bool:
|
||||
return wanted is None or bool(lines & wanted)
|
||||
|
||||
|
||||
def python_comments(path: str, tokens: list[tokenize.TokenInfo], wanted: set[int] | None) -> list[Hit]:
|
||||
return [
|
||||
Hit(path, token.start[0], "python", "comment", defaults.short(token.string), bool(DIRECTIVE.search(token.string)))
|
||||
for token in tokens
|
||||
if token.type == tokenize.COMMENT
|
||||
and not (token.start[0] == 1 and token.string.startswith("#!"))
|
||||
and touched({token.start[0]}, wanted)
|
||||
]
|
||||
|
||||
|
||||
def python_docstrings(path: str, tree: ast.AST, wanted: set[int] | None) -> list[Hit]:
|
||||
nodes = [
|
||||
node
|
||||
for node in ast.walk(tree)
|
||||
if isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant) and isinstance(node.value.value, str)
|
||||
]
|
||||
return [
|
||||
Hit(path, node.lineno, "python", "docstring", defaults.short(node.value.value), False)
|
||||
for node in nodes
|
||||
if touched(set(range(node.lineno, (node.end_lineno or node.lineno) + 1)), wanted)
|
||||
]
|
||||
|
||||
|
||||
def scan_python(path: str, text: str, wanted: set[int] | None) -> list[Hit]:
|
||||
try:
|
||||
tree = ast.parse(text, filename=path)
|
||||
tokens = list(tokenize.generate_tokens(io.StringIO(text).readline))
|
||||
except (SyntaxError, ValueError, tokenize.TokenError) as error:
|
||||
raise defaults.ScanError(f"{path}: unparseable Python: {error}") from error
|
||||
return python_comments(path, tokens, wanted) + python_docstrings(path, tree, wanted)
|
||||
|
||||
|
||||
def masked_pattern(langs: list[tuple[str, str, str]]) -> re.Pattern[str]:
|
||||
strings = "|".join(pattern for _, pattern, _ in langs if pattern)
|
||||
source = "(?P<comment>" + "|".join(pattern for _, _, pattern in langs) + ")"
|
||||
return re.compile(source + (f"|(?P<string>{strings})" if strings else ""), re.MULTILINE | re.DOTALL)
|
||||
|
||||
|
||||
def scan_masked(path: str, text: str, langs: list[tuple[str, str, str]], wanted: set[int] | None) -> list[Hit]:
|
||||
_, spans = defaults.mask(text, masked_pattern(langs))
|
||||
starts = [0] + [newline.end() for newline in re.finditer("\n", text)]
|
||||
language = "+".join(lang for lang, _, _ in langs)
|
||||
hits = []
|
||||
for begin, end in spans:
|
||||
body = text[begin:end]
|
||||
if begin == 0 and body.startswith("#!"):
|
||||
continue
|
||||
first = bisect.bisect_right(starts, begin)
|
||||
lines = set(range(first, bisect.bisect_right(starts, max(begin, end - 1)) + 1))
|
||||
if touched(lines, wanted):
|
||||
hits.append(Hit(path, first, language, "comment", defaults.short(body), bool(DIRECTIVE.search(body))))
|
||||
return hits
|
||||
|
||||
|
||||
def scan(path: str, text: str, wanted: set[int] | None) -> list[Hit]:
|
||||
if python_lang(path):
|
||||
return scan_python(path, text, wanted)
|
||||
langs = patterns(path)
|
||||
return scan_masked(path, text, langs, wanted) if langs else []
|
||||
|
||||
|
||||
def changed_targets(root: Path, staged: bool) -> dict[str, tuple[str, set[int] | None]]:
|
||||
targets: dict[str, tuple[str, set[int] | None]] = {}
|
||||
for path, lines in defaults.added_lines(root, staged).items():
|
||||
if known(path):
|
||||
text = defaults.git(root, "cat-file", "blob", f":{path}") if staged else defaults.read(root / path)
|
||||
targets[path] = (text, lines)
|
||||
return targets
|
||||
|
||||
|
||||
def file_targets(repo: Path, root: Path, files: list[str]) -> dict[str, tuple[str, set[int] | None]]:
|
||||
targets: dict[str, tuple[str, set[int] | None]] = {}
|
||||
for name in files:
|
||||
target = (repo / name).resolve()
|
||||
if not target.is_file():
|
||||
raise defaults.ScanError(f"{name}: no such file")
|
||||
shown = target.relative_to(root).as_posix() if target.is_relative_to(root) else str(target)
|
||||
if known(shown):
|
||||
targets[shown] = (defaults.read(target), None)
|
||||
else:
|
||||
print(f"comments: {shown}: no comment syntax known for this file type", file=sys.stderr)
|
||||
return targets
|
||||
|
||||
|
||||
def collect(repo: Path, staged: bool, files: list[str]) -> list[Hit]:
|
||||
if not repo.is_dir():
|
||||
raise defaults.ScanError(f"{repo} is not a directory")
|
||||
root = Path(defaults.git(repo, "rev-parse", "--show-toplevel").strip()).resolve()
|
||||
targets = changed_targets(root, staged) | file_targets(repo, root, files)
|
||||
hits = [hit for path, (text, wanted) in targets.items() for hit in scan(path, text, wanted)]
|
||||
return sorted(hits, key=lambda hit: (hit.file, hit.line, hit.kind))
|
||||
|
||||
|
||||
def main() -> int:
|
||||
parser = argparse.ArgumentParser(description=__doc__.splitlines()[0])
|
||||
parser.add_argument("--repo", required=True)
|
||||
parser.add_argument("--staged", action="store_true")
|
||||
parser.add_argument("--file", action="append", default=[])
|
||||
parser.add_argument("--json", action="store_true")
|
||||
args = parser.parse_args()
|
||||
try:
|
||||
hits = collect(Path(args.repo), args.staged, args.file)
|
||||
except defaults.ScanError as error:
|
||||
print(f"comments: {error}", file=sys.stderr)
|
||||
return 2
|
||||
if args.json:
|
||||
print(json.dumps([asdict(hit) for hit in hits], indent=1))
|
||||
else:
|
||||
for hit in hits:
|
||||
row = [f"{hit.file}:{hit.line}", hit.language, hit.kind, hit.text] + (["directive"] if hit.directive else [])
|
||||
print("\t".join(row))
|
||||
return 1 if any(not hit.directive for hit in hits) else 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -0,0 +1,128 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[1]
|
||||
SCRIPT = REPO_ROOT / "skills" / "kvb-code-clean" / "scripts" / "comments.py"
|
||||
BASE = {
|
||||
"casc.yaml.j2": "# Header kept from the base.\njenkins:\n url: x\n",
|
||||
"app.py": "def old():\n return 1\n",
|
||||
"run.sh": "#!/bin/bash\necho old\n",
|
||||
}
|
||||
UNSTAGED = {
|
||||
"casc.yaml.j2": BASE["casc.yaml.j2"]
|
||||
+ "{# Without an override the plugin requests every scope. #}\n"
|
||||
+ " scopes: \"openid\"\n"
|
||||
+ " # a yaml narration line\n"
|
||||
+ ' note: "{{ value }}"\n',
|
||||
"app.py": BASE["app.py"]
|
||||
+ "\n\ndef new(a):\n"
|
||||
+ ' """Return a."""\n'
|
||||
+ " # explain the next line\n"
|
||||
+ " return a # noqa: E501\n",
|
||||
"run.sh": BASE["run.sh"]
|
||||
+ "# shellcheck disable=SC2086\n"
|
||||
+ 'echo "#not a comment" $X\n',
|
||||
}
|
||||
STAGED = {
|
||||
"style.css": "a { color: red; }\n/* palette narration */\n",
|
||||
"page.html.j2": "<p>{{ title }}</p>\n<!-- layout note -->\n{# jinja note #}\n",
|
||||
"Dockerfile": "FROM debian\n# install the tools\nRUN true\n",
|
||||
}
|
||||
UNSTAGED_HITS = [
|
||||
("app.py:6", "python", "docstring", False),
|
||||
("app.py:7", "python", "comment", False),
|
||||
("app.py:8", "python", "comment", True),
|
||||
("casc.yaml.j2:4", "jinja+yaml", "comment", False),
|
||||
("casc.yaml.j2:6", "jinja+yaml", "comment", False),
|
||||
("run.sh:3", "shell", "comment", True),
|
||||
]
|
||||
STAGED_HITS = [
|
||||
("Dockerfile:2", "dockerfile", "comment", False),
|
||||
("page.html.j2:2", "jinja+html", "comment", False),
|
||||
("page.html.j2:3", "jinja+html", "comment", False),
|
||||
("style.css:2", "css", "comment", False),
|
||||
]
|
||||
|
||||
|
||||
class TestComments(unittest.TestCase):
|
||||
def setUp(self):
|
||||
tmp = tempfile.TemporaryDirectory()
|
||||
self.addCleanup(tmp.cleanup)
|
||||
self.repo = Path(tmp.name) / "repo"
|
||||
self.repo.mkdir()
|
||||
self.env = {
|
||||
"PATH": "/usr/bin:/bin",
|
||||
"HOME": tmp.name,
|
||||
"GIT_CONFIG_NOSYSTEM": "1",
|
||||
"GIT_CEILING_DIRECTORIES": tmp.name,
|
||||
"GIT_AUTHOR_NAME": "t",
|
||||
"GIT_AUTHOR_EMAIL": "t@example.org",
|
||||
"GIT_COMMITTER_NAME": "t",
|
||||
"GIT_COMMITTER_EMAIL": "t@example.org",
|
||||
}
|
||||
self._git("init", "-q", "-b", "main")
|
||||
self._write(BASE)
|
||||
self._git("add", ".")
|
||||
self._git("commit", "-q", "-m", "base")
|
||||
|
||||
def _git(self, *args: str) -> str:
|
||||
return subprocess.run(
|
||||
["git", *args], cwd=self.repo, env=self.env, capture_output=True, text=True, check=True
|
||||
).stdout
|
||||
|
||||
def _write(self, files: dict[str, str]) -> None:
|
||||
for name, text in files.items():
|
||||
(self.repo / name).write_text(text, encoding="utf-8")
|
||||
|
||||
def _change(self) -> None:
|
||||
self._write(STAGED)
|
||||
self._git("add", *STAGED)
|
||||
self._write(UNSTAGED)
|
||||
|
||||
def _hits(self, *args: str) -> tuple[int, list[tuple]]:
|
||||
result = subprocess.run(
|
||||
[sys.executable, str(SCRIPT), "--repo", str(self.repo), "--json", *args],
|
||||
env=self.env,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
)
|
||||
self.assertIn(result.returncode, (0, 1), result.stderr)
|
||||
rows = json.loads(result.stdout)
|
||||
return result.returncode, sorted(
|
||||
(f"{r['file']}:{r['line']}", r["language"], r["kind"], r["directive"]) for r in rows
|
||||
)
|
||||
|
||||
def test_worktree_reports_every_added_comment_and_nothing_from_the_base(self):
|
||||
self._change()
|
||||
code, hits = self._hits()
|
||||
self.assertEqual(code, 1)
|
||||
self.assertEqual(hits, sorted(UNSTAGED_HITS + STAGED_HITS))
|
||||
|
||||
def test_staged_reads_the_index_version(self):
|
||||
self._change()
|
||||
code, hits = self._hits("--staged")
|
||||
self.assertEqual(code, 1)
|
||||
self.assertEqual(hits, sorted(STAGED_HITS))
|
||||
|
||||
def test_directives_alone_exit_zero(self):
|
||||
self._write({"run.sh": BASE["run.sh"] + "# shellcheck disable=SC2086\necho $X\n"})
|
||||
code, hits = self._hits()
|
||||
self.assertEqual(code, 0)
|
||||
self.assertEqual(hits, [("run.sh:3", "shell", "comment", True)])
|
||||
|
||||
def test_file_scans_an_untracked_template_whole(self):
|
||||
(self.repo / "new.yml.j2").write_text("a: 1\n{# narration #}\n", encoding="utf-8")
|
||||
code, hits = self._hits("--file", "new.yml.j2")
|
||||
self.assertEqual(code, 1)
|
||||
self.assertEqual(hits, [("new.yml.j2:2", "jinja+yaml", "comment", False)])
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in new issue
Block a user