Skip to content

Commit

Permalink
deprecate glob --not in favor of glob --exclude (#10827)
Browse files Browse the repository at this point in the history
# Description
looking at the [Wax documentation about
`wax::Walk.not`](https://docs.rs/wax/latest/wax/struct.Walk.html#examples),
especially
> therefore does not read directory trees from the file system when a
directory matches an [exhaustive glob
expression](https://docs.rs/wax/latest/wax/trait.Pattern.html#tymethod.is_exhaustive)

> **Important**
> in the following of this PR description, i talk about *pruning* and a
`--prune` option, but this has been changed to *exclusion* and
`--exclude` after a discussion with @fdncred.

this looks like a *pruning* operation to me, right? 😮 
i wanted to make the `glob` option `--not` clearer about that, because
>   -n, --not <List(String)> - Patterns to exclude from the results

from `help glob` is not very explicit about whether the search is pruned
when entering a directory matching a pattern in `--not` or just removing
it from the output 😕

## changelog
this PR proposes to rename the `glob --not` option to `glob --prune` and
make it's documentation more explicit 😋

## benchmarking
to support the *pruning* behaviour put forward above, i've run a
benchmark
1. define two closures to compare the behaviour between removing
patterns manually or using `--not`
```nushell
let where = {
    [.*/\.local/.*, .*/documents/.*, .*/\.config/.*]
        | reduce --fold (glob **) {|pat, acc| $acc | where $it !~ $pat}
        | length
}
```
```nushell
let not = { glob ** --not [**/.local/**, **/documents/**, **/.config/**] | length }
```
2. run the two to make sure they give similar results
```nushell
> do $where
33424
```
```nushell
> do $not
33420
```
👌 
3. measure the performance
```nushell
use std bench
```
```nushell
> bench --verbose --pretty --rounds 25 $not
44ms 52µs 285ns +/- 977µs 571ns
```
```nushell
> bench --verbose --pretty --rounds 5 $where
1sec 250ms 187µs 99ns +/- 8ms 538µs 57ns
```

👉 we can see that the results are (almost) the same but
`--not` is much faster, looks like pruning 😋

# User-Facing Changes
- `--not` will give a warning message but still work
- `--prune` will work just as `--not` without warning and with a more
explicit doc
- `--prune` and `--not` at the same time will give an error

# Tests + Formatting
this PR fixes the examples of `glob` using the `--not` option.

# After Submitting
prepare the removal PR and mention in release notes.
  • Loading branch information
amtoine committed Oct 25, 2023
1 parent e2fb0e5 commit 7ac5a01
Showing 1 changed file with 39 additions and 4 deletions.
43 changes: 39 additions & 4 deletions crates/nu-command/src/filesystem/glob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,15 @@ impl Command for Glob {
.named(
"not",
SyntaxShape::List(Box::new(SyntaxShape::String)),
"Patterns to exclude from the results",
"DEPRECATED OPTION: Patterns to exclude from the results",
Some('n'),
)
.named(
"exclude",
SyntaxShape::List(Box::new(SyntaxShape::String)),
"Patterns to exclude from the search: `glob` will not walk the inside of directories matching the excluded patterns.",
Some('e'),
)
.category(Category::FileSystem)
}

Expand Down Expand Up @@ -111,12 +117,12 @@ impl Command for Glob {
},
Example {
description: "Search for files named tsconfig.json that are not in node_modules directories",
example: r#"glob **/tsconfig.json --not [**/node_modules/**]"#,
example: r#"glob **/tsconfig.json --exclude [**/node_modules/**]"#,
result: None,
},
Example {
description: "Search for all files that are not in the target nor .git directories",
example: r#"glob **/* --not [**/target/** **/.git/** */]"#,
example: r#"glob **/* --exclude [**/target/** **/.git/** */]"#,
result: None,
},
]
Expand All @@ -141,8 +147,37 @@ impl Command for Glob {
let no_files = call.has_flag("no-file");
let no_symlinks = call.has_flag("no-symlink");

if call.has_flag("not") {
nu_protocol::report_error_new(
engine_state,
&ShellError::GenericError(
"Deprecated option".into(),
"`glob --not {list<string>}` is deprecated and will be removed in 0.88.".into(),
Some(call.head),
Some("Please use `glob --exclude {list<string>}` instead.".into()),
vec![],
),
);
}

let not_flag: Option<Value> = call.get_flag(engine_state, stack, "not")?;
let (not_patterns, not_pattern_span): (Vec<String>, Span) = match not_flag {
let exclude_flag: Option<Value> = call.get_flag(engine_state, stack, "exclude")?;

let paths_to_exclude = match (not_flag, exclude_flag) {
(Some(not_flag), Some(exclude_flag)) => {
return Err(ShellError::IncompatibleParameters {
left_message: "Cannot pass --not".into(),
left_span: not_flag.span(),
right_message: "and --exclude".into(),
right_span: exclude_flag.span(),
})
}
(Some(not_flag), None) => Some(not_flag),
(None, Some(exclude_flag)) => Some(exclude_flag),
(None, None) => None,
};

let (not_patterns, not_pattern_span): (Vec<String>, Span) = match paths_to_exclude {
None => (vec![], span),
Some(f) => {
let pat_span = f.span();
Expand Down

0 comments on commit 7ac5a01

Please sign in to comment.