Support glob patterns in the exclude config#901
Conversation
| context.touch(&nested); | ||
|
|
||
| let mut excluded = HashSet::new(); | ||
| excluded.insert(context.absolute_path_to("**/fixtures")); |
There was a problem hiding this comment.
Does this insert a pattern or a path 🤔
And should we have 2 tests exercising both branches of
Ok(canonical) => Pattern::escape(&canonical.to_string_lossy()),
Err(_) => path.to_string_lossy().into_owned(),There was a problem hiding this comment.
Does this insert a pattern or a path 🤔
Yeah, it was ambiguous, the type was lying to us. Exclude entries can be glob patterns so they are Box<str> now instead of PathBuf.
should we have 2 tests exercising both branches
I changed the logic so this is a bit different now, but both branches were covered. The Err branch was the only one needing a test, as the Ok branch was the pre-existing behaviour.
alexcrocha
force-pushed
the
ar-glob-pattern-config-support
branch
2 times, most recently
from
488ee40 to
39db7c5
Compare
Exclude entries must be able to hold glob patterns, which are not filesystem paths
Representing exclusions as patterns allows us to support exclusion matching with globs without reworking the traversal or the match check.
A glob is recognized by its metacharacters and kept as a pattern, while concrete paths canonicalize and match literally
alexcrocha
force-pushed
the
ar-glob-pattern-config-support
branch
from
39db7c5 to
6e96728
Compare
| self.workspace_path | ||
| .join(&**entry) | ||
| .to_string_lossy() | ||
| .replace(std::path::MAIN_SEPARATOR, "/") |
There was a problem hiding this comment.
Why are we substituting the separator here? Note that this PR changes excluded_paths into excluded_glob_patterns. For file paths, backslashes are valid (on Windows), but not for glob patterns (which always use forward slashes).
| if entry.contains(['*', '?', '[']) { | ||
| Pattern::new(entry).ok() | ||
| } else { | ||
| let canonical = fs::canonicalize(entry).ok()?; |
There was a problem hiding this comment.
Did you find that treating all entries as glob patterns was too slow? Do we need the two separate paths?
Also, I think there's potential for this to be slightly confusing. Consider this example:
project_path/
my_symlink -> some_other_path
Trying to exclude the same directory using a pattern vs a path results in different behaviour:
# rubydex.toml
# This config path gets canonicalized to `some_other_path`, which then
# gets excluded since we also canonicalize the paths being compared
exclude = ["my_symlink"]
# This pattern does not get canonicalized since you cannot do it for a glob
# pattern, which means we compare if `some_other_path` matches `**/my_symlink`.
#
# It does not and so we fail to exclude
exclude = ["**/my_symlink/"]
I would vote for us to do the following:
- Stop following symlinks (both in the config and in the listing). If someone created a symlink in their project, I would assume they'd prefer dealing with the symlink they created rather than the destination path
- Treat everything as glob patterns
3 participants
This PR lets
excludeentries be glob patterns (e.g.**/fixtures), not just literal paths. Globs are matched during file discovery.Excludes are canonicalized to resolve symlinks, but a glob is not a real path and cannot be canonicalized. So canonicalization failure is what tells the two apart: an entry that canonicalizes is concrete and matched literally, one that fails is used as a glob.