From 7ac5a01e2f5020a1295697edab727cd62a5844b7 Mon Sep 17 00:00:00 2001 From: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Wed, 25 Oct 2023 17:11:57 +0200 Subject: [PATCH] deprecate `glob --not` in favor of `glob --exclude` (#10827) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 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? :open_mouth: i wanted to make the `glob` option `--not` clearer about that, because > -n, --not - 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 :confused: ## changelog this PR proposes to rename the `glob --not` option to `glob --prune` and make it's documentation more explicit :yum: ## 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 ``` :ok_hand: 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 ``` :point_right: we can see that the results are (almost) the same but `--not` is much faster, looks like pruning :yum: # 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. --- crates/nu-command/src/filesystem/glob.rs | 43 +++++++++++++++++++++--- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/crates/nu-command/src/filesystem/glob.rs b/crates/nu-command/src/filesystem/glob.rs index 8676ab9b1..9b1002ba7 100644 --- a/crates/nu-command/src/filesystem/glob.rs +++ b/crates/nu-command/src/filesystem/glob.rs @@ -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) } @@ -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, }, ] @@ -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}` is deprecated and will be removed in 0.88.".into(), + Some(call.head), + Some("Please use `glob --exclude {list}` instead.".into()), + vec![], + ), + ); + } + let not_flag: Option = call.get_flag(engine_state, stack, "not")?; - let (not_patterns, not_pattern_span): (Vec, Span) = match not_flag { + let exclude_flag: Option = 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, Span) = match paths_to_exclude { None => (vec![], span), Some(f) => { let pat_span = f.span();