Fix --erase ignore precedence under default-ignored ancestors

[Copilot]

--erase matches were being dropped when the target directory lived under default-ignored ancestors (e.g. .tox, .venv) because erase filtering reused ancestor-aware global ignore rules. This made explicit targets silently no-op for patterns like **/*.

• Separate explicit vs default ignore state

  • --ignore now parses with default=None.
  • Added args.explicit_ignore for user-provided ignore patterns only.
  • args.ignore remains the traversal/debris list: defaults + explicit patterns (deduplicated, order-preserving).

• Apply erase-specific filtering semantics

  • remove_freeform_targets() now passes only explicit_ignore_patterns to erase filtering.
  • delete_filesystem_objects() accepts ignore_patterns; when provided, erase filtering uses that set instead of global defaults.
  • Existing internal behavior is preserved when ignore_patterns is omitted (fallback to Runner.ignore).

• Regression coverage for the reported behavior

  • Added tests proving:
    • erase works inside an explicitly targeted directory under .tox.
    • explicit --ignore still excludes specific erase matches.
    • CLI still exposes default ignore behavior when --ignore is not passed.

# cli.py
args.explicit_ignore = args.ignore if args.ignore is not None else []
args.ignore = list(dict.fromkeys([*ignore_default_items, *args.explicit_ignore]))

# main.py
remove_freeform_targets(
    dir_path,
    args.erase,
    args.yes,
    args.dry_run,
    explicit_ignore_patterns=getattr(args, 'explicit_ignore', []),
)

[bittner]

Code Review — Fix --erase ignore precedence under default-ignored ancestors

I reviewed the PR against issue #122, read the surrounding code, ran the test suite, lint, format, and type checks on the PR branch, and reproduced the behavior change manually. Verdict: correct and faithful to the issue's design — recommend merge after a few small follow-ups.

Overview

The PR fixes #122 by separating two concepts that were previously conflated:

• args.ignore — defaults + explicit patterns → still used for traversal (bytecode, --debris, --folders, --git-clean).
• args.explicit_ignore — user-provided --ignore patterns only → used for --erase filtering.

This implements the issue's precedence ladder exactly: defaults < directory positional < explicit --ignore.

Correctness — verified ✅

I traced every case in the issue's table and confirmed the PR is correct:

Command	Expected	PR behavior
pyclean DIR --erase '**/*'	deletes everything in DIR	explicit_ignore=[] → ignore_patterns=[] → if ignore_patterns: false → no filtering ✅
pyclean DIR --erase '**/*' --ignore FOO	deletes all except FOO	filters by ['FOO'] only ✅
pyclean DIR --erase '**/*' --ignore DIR	no-op	every match has DIR as ancestor → all filtered ✅

Other things I checked and confirmed sound:

• Traversal is untouched. Runner.ignore is still defaults + explicit, so bytecode/debris/folders/git-clean keep avoiding .git/.tox/etc. — no regression there (the issue's biggest worry).
• None-fallback equivalence. When ignore_patterns is None, path_is_ignored(n, Runner.ignore) is identical to the old Runner.is_ignored(n). Debris's direct delete_filesystem_objects(directory, pattern) call (debris.py:96) is unaffected. ✅
• argparse default=None + action='extend'. Correct: _copy_items(None) yields [], so an explicit --ignore produces just the user's list; dict.fromkeys then rebuilds args.ignore defaults-first, deduped, order-preserving. ✅
• from __future__ import annotations in erase.py is required, not cosmetic — directory: Path | str (PEP 604) would raise TypeError at def-time on Python 3.9, which tox still targets. The PR got this right.
• All 58 tests pass, plus lint, format, and ty type checks — all green.

UX / behavior-change assessment

I reproduced the one behavior change that affects existing users, in /tmp:

pyclean proj --erase '**/*' --yes      # proj/ contains .git/, .venv/, src/
  main (v3.6.0):  Total 1 files, 1 directories removed   (.git + .venv survive)
  PR #123:        Total 4 files, 5 directories removed   (.git + .venv wiped)

So --erase no longer gets the default-ignore safety net — pyclean . --erase '**/*' now wipes .git too.

This is intended — #122 explicitly designs it ("traverse DIR for better or worse and delete everything therein") — and arguably more intuitive, not less. The v3.6.0 behavior is the real surprise: --erase '**/*' reports success while silently leaving directories in place — that is #122. Making **/* mean **/* is the more logical contract, and the README already warns prominently that --erase is "potentially dangerous."

That said, it's a real change for everyone, not just the .tox-ancestor case — worth a conscious nod in the release notes.

Suggested follow-ups (none blocking)

1. Fix --ignore flag not suppressing --erase targets #119 regression test no longer guards what it was written for. test_erase_ignored_dir_produces_no_output calls remove_freeform_targets(...) without explicit_ignore_patterns, so it now exercises the None→Runner.ignore fallback — a path main.py never takes (it always passes explicit_ignore_patterns=). The test passes, but it would no longer catch a break in the explicit-ignore wiring. Pass explicit_ignore_patterns=['foo'] to test the production path. (Coverage isn't lost overall — the new test_erase_applies_explicit_ignore_on_top_of_defaults exercises the real path via pyclean.main.pyclean — but the old test is now misleading.)

2. None vs [] dual semantics is a latent footgun. explicit_ignore_patterns=None means "fall back to defaults+explicit"; [] means "no filtering." Only test code relies on None. Consider defaulting remove_freeform_targets's parameter to [] (matching production reality) and confining the None-fallback to delete_filesystem_objects for debris — this resolves debpython breaks Python 2<->3 compatibility #1 cleanly too.

3. Add a test for issue case 3 (--erase + --ignore DIR → full no-op). The PR covers cases 1 and 2 but not the "user can still protect the named directory" guarantee.

4. Docs: remove_freeform_targets's docstring wasn't updated for the new explicit_ignore_patterns parameter (delete_filesystem_objects's was). The README --erase section (lines 220–243) doesn't mention --ignore interaction at all — a one-liner ("--erase is filtered only by explicit --ignore patterns; the built-in default ignore list does not restrict --erase") would set the right expectation.

5. Minor: the 8-element default ignore list is hardcoded in three new test spots. The two erase tests don't actually depend on its exact contents — they only need .tox as an ancestor and explicit_ignore=[] — so those ignore=[...] blocks could be trimmed. (test_ignore_defaults_without_option legitimately asserts the full list.)

Bottom line: the implementation is correct, faithfully matches the issue's design, preserves traversal behavior, and passes all checks. The follow-ups are about test fidelity and documentation, not correctness — safe to merge with item 1 addressed.

[Copilot]

Pull request overview

Fixes --erase behavior so explicit erase targets aren’t accidentally suppressed by default ignore ancestors (e.g. .tox, .venv), while preserving default ignore behavior for traversal/debris cleanup.

Changes:

• Split ignore handling into explicit_ignore (user-provided only) vs ignore (defaults + explicit, deduped) in the CLI.
• Adjust erase filtering so --erase uses only explicit ignore patterns (and not built-in defaults) when deciding what erase matches to drop.
• Add regression tests covering erase under default-ignored ancestors and precedence interactions with --ignore.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pyclean/cli.py	Builds args.explicit_ignore and recomputes args.ignore as defaults + explicit patterns.
pyclean/main.py	Wires explicit_ignore_patterns into remove_freeform_targets() for erase semantics.
pyclean/erase.py	Adds ignore_patterns plumbing and applies explicit-ignore-only filtering to erase matches.
README.rst	Documents that --erase filtering is constrained only by explicit --ignore patterns.
tests/test_cli.py	Verifies explicit_ignore and default ignore behavior when --ignore is omitted.
tests/test_erase.py	Adds regression coverage for erase precedence under default-ignored ancestors and explicit ignores.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.