Skip to content

Allow using globs in excluded paths#32

Open
yozhgoor wants to merge 5 commits intomainfrom
glob-excluded-paths
Open

Allow using globs in excluded paths#32
yozhgoor wants to merge 5 commits intomainfrom
glob-excluded-paths

Conversation

@yozhgoor
Copy link
Copy Markdown
Member

@yozhgoor yozhgoor commented Apr 14, 2026

Closes #31

@yozhgoor yozhgoor requested a review from cecton April 14, 2026 23:46
Copy link
Copy Markdown
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall: Looks solid, with a few issues to address.


Bug: Double space in error message (src/lib.rs:342)

.with_context(|| format!("failed to  `{}`", path.display()))?;

There's a double space and the message is also incomplete — it says "failed to \...`"with no verb. Should probably be"failed to canonicalize `{}`"`.


Design concern: private exclude_globs / workspace_exclude_globs fields accessed in tests

The new fields exclude_globs and workspace_exclude_globs are private, but the tests directly manipulate them. Users going through the public builder API (exclude_path, exclude_workspace_path) will have globs parsed at run() time, which is fine — but the tests bypass this entirely by accessing private fields. The tests should ideally test through the public path (e.g. by populating exclude_paths / workspace_exclude_paths and exercising the parsing logic in run()).


Missing integration test

There's no test verifying that a glob string passed via exclude_paths or workspace_exclude_paths (the public API / CLI path) actually gets parsed into exclude_globs / workspace_exclude_globs and correctly excludes paths end-to-end.


workspace_exclude_paths globs are not resolved relative to workspace root (src/lib.rs:348–356)

For workspace_exclude_paths, globs are compiled as-is without prepending the workspace root. This is consistent with is_excluded_path stripping the workspace prefix before matching, so the behaviour is correct — just worth a comment to make the intent explicit, since it differs from how exclude_paths globs are handled.


Unrelated change: edition = "2024" bump (Cargo.toml)

The PR bumps edition from 2021 to 2024 and rust-version from 1.78 to 1.85.1. This is unrelated to the feature and should be a separate PR or at least explicitly called out in the description.


Minor: is_glob_pattern false positives on Windows

[ is a valid character in Windows filenames, so s.contains('[') could misclassify a plain path as a glob on Windows. Low priority, but worth noting.

@yozhgoor yozhgoor requested a review from cecton April 15, 2026 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow using globs in exluded_paths

3 participants