Honor .gitignore and accept gitignore syntax in ignored_paths - #366
Honor .gitignore and accept gitignore syntax in ignored_paths#366steve-downey wants to merge 1 commit into
Conversation
…paths The ignore list only did exact and prefix matching, so a repository had to repeat in .beman-tidy.yaml what its .gitignore already said, and could not express a pattern at all. Ignore matching now goes through an IgnoreMatcher built on pathspec, and every source uses .gitignore pattern syntax: the built-in ignores, the 'ignored_paths' entries and the .gitignore files of the repository itself. Nested .gitignore files apply relative to their own directory, deepest first, and one inside an ignored directory is never read - as in git. Sources are consulted from 'ignored_paths' down to the built-in ignores, so a '!' entry in the configuration re-includes a path that a .gitignore covers. As in git, that cannot rescue a path below an ignored directory. README.md and LICENSE stay mandatory: a .gitignore matching them warns instead of skipping the checks. Set 'use_gitignore: false' to opt out. Checked against bemanproject/exemplar: the reported checks and coverage are unchanged.
|
A very good improvement for me. |
neatudarius
left a comment
neatudarius
left a comment
There was a problem hiding this comment.
@steve-downey , thanks for contribution! LGTM, few small things to address in this PR or follow-up one (check next comments).
Before merge, please after your branch with latest main after #370 and put the label run-on-all-libraries. This way you will start an on-demand job across the org to make sure your changes does not break any library.
| @@ -0,0 +1,253 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Minor, non-blocking observations
-
_as_matchersorts sets/frozenset inputs (file.py _as_matcher) before building a spec. Sorting alphabetically would scramble negation order if a caller ever passed a set containing a !pattern (since ! sorts before letters) — negation order matters in gitignore semantics. Today this is unreachable (the only real caller passes an IgnoreMatcher directly, and the only literal set is the negation-free default ignore list), so it's not an active bug — just a footgun if someone starts passing raw patterns with negations through the old set-based API. Worth a one-line comment or narrowing the type accepted, not a blocker. -
.gitignorefiles are walked twice (once to build layers, once by each file generator's own os.walk). Fine for Beman-sized repos; not worth optimizing now. -
Two widening behavior changes are called out explicitly in the PR description (slashless ignored_paths entries now match at any depth, build/ now matches nested build dirs) — these are correct per gitignore semantics but are real behavior changes for existing configs. The PR already verified byte-identical output against a fresh exemplar clone, which is the right check.
There was a problem hiding this comment.
sorts sets/frozenset inputs (file.py _as_matcher) before building a spec.
well spotted. I did fix one sort problem, and missed this. Possibly because it's not reached. But I agree it's the sort of terrible surprise someone could find later.
real behavior changes for existing configs.
are any configs under the beman project org using that in an interesting way.
That might be something answered by the #370 PR after I rebase it.
Possibly worth a major version bump, though?
3 participants
Summary
beman-tidyignored only what.beman-tidy.yamllisted, matching exactly and by prefix. A repository had to repeat in its configuration what its.gitignorealready said, and could not express a pattern at all.This change routes ignore matching through an
IgnoreMatcherbuilt onpathspec, so every ignore source speaks.gitignorepattern syntax:.gitignorefiles are honored, including nested ones, each relative to its own directoryignored_pathsentries accept wildcards,**, leading/anchoring, trailing/for directories, and!to re-includebuild/,__pycache__/, IDE directories, ...) are patterns tooSources are consulted from
ignored_paths, through the.gitignorefiles deepest-first, down to the built-in ignores; the first matching pattern decides. A!entry in the configuration therefore re-includes a path that a.gitignorecovers.Behavior notes
use_gitignore: falsein.beman-tidy.yaml.README.mdandLICENSEstay mandatory.validate_configalready rejects them inignored_paths; a.gitignorematching them is not a configuration error, so it warns and the checks still run.ignored_paths. Withgenerated/in a.gitignore, no!generated/api.hppworks — the repository has to ignoregenerated/*instead. Relaxing this would desynchronizeis_ignored()from the directory pruning in the tree walk.ignored_pathsentry such asdocsnow matches at any depth rather than only at the root, and the built-inbuild/now matches nested build directories..venv/added to the built-in ignores. The defaults now also bound how much of the tree is walked when collecting.gitignorefiles.Verification
ruff checkclean.bemanproject/exemplar: reported checks and coverage are byte-identical before and after this change..cache/scratch.cppfile — matched by exemplar's/.cacheentry — is reported before the change and skipped after it.Changes
beman_tidy/lib/utils/ignore.py(new):IgnoreMatcher, layered specs,.gitignorecollection, per-root cachebeman_tidy/lib/utils/config.py:get_ignore_matcher()replacesget_ignores();use_gitignorevalidated as a booleanbeman_tidy/lib/utils/file.py: the duplicate prefix matcher is gone; both file generators use the shared matchertests/lib/utils/test_config_gitignore.py(new): 23 tests covering pattern syntax, nesting, precedence, the mandatory files and the generatorsREADME.md: configuration section and two FAQ entriespyproject.toml/uv.lock/pylock.toml:pathspec>=0.12.1, lockfiles regenerated perdocs/dev-guide.md🤖 Generated with Claude Code