Conversation
cecton
left a comment
There was a problem hiding this comment.
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.
Closes #31