Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate glob --not in favor of glob --exclude #10827

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Oct 24, 2023

Description

looking at the Wax documentation about wax::Walk.not, especially

therefore does not read directory trees from the file system when a directory matches an exhaustive glob expression

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
let where = {
    [.*/\.local/.*, .*/documents/.*, .*/\.config/.*]
        | reduce --fold (glob **) {|pat, acc| $acc | where $it !~ $pat}
        | length
}
let not = { glob ** --not [**/.local/**, **/documents/**, **/.config/**] | length }
  1. run the two to make sure they give similar results
> do $where
33424
> do $not
33420

👌
3. measure the performance

use std bench
> bench --verbose --pretty --rounds 25 $not
44ms 52µs 285ns +/- 977µs 571ns
> 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.

@amtoine amtoine added pr:release-note-mention Addition/Improvement to be mentioned in the release notes deprecation Related to the deprecation of commands labels Oct 24, 2023
@fdncred
Copy link
Collaborator

fdncred commented Oct 24, 2023

I like --not better than --prune because not is more intuitive and we're not gardening :), plus that's the function's name in wax. I realize fd uses --prune but that doesn't mean we have to. To the novice, --prune may be a bizarre parameter name.

@amtoine
Copy link
Member Author

amtoine commented Oct 25, 2023

I like --not better than --prune because not is more intuitive and we're not gardening :), plus that's the function's name in wax. I realize fd uses --prune but that doesn't mean we have to. To the novice, --prune may be a bizarre parameter name.

i do not use fd really, so this was not inspired directly by fd --prune 😉
but this still is a valid point, i agree 👍
to me, a filesystem is a tree, so it makes sense to say prune when a branch of the filesystem tree is removed.
but i do not want the non-computer-science folks to be confused about that 😌

i've talked to 3 dev persons and one non-dev to have some insight and here are four ideas

  • use --prune
  • use --exclude-from-search or --skip
  • simply update the doc of --not
  • leave glob as-is

i could live with option 3 but

  • all four of them have voted for either --exclude-from-search or --skip, and my personal favorite is --exclude-from-search.
  • all four of them have voted for not --not because it does not show that the matching patterns are removed from the search, so i wouldn't vote for the last option as well 😋

@fdncred
Copy link
Collaborator

fdncred commented Oct 25, 2023

I'd vote for --exclude probably. --exclude-from-search seems a bit too verbose.

@amtoine
Copy link
Member Author

amtoine commented Oct 25, 2023

I'd vote for --exclude probably. --exclude-from-search seems a bit too verbose.

let's go with --exclude and the updated doc 🙏

@amtoine amtoine changed the title deprecate glob --not in favor of glob --prune deprecate glob --not in favor of glob --exclude Oct 25, 2023
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

looks good

@amtoine
Copy link
Member Author

amtoine commented Oct 25, 2023

let's go with this then, hopefully a bit clearer 😇

@amtoine amtoine merged commit 7ac5a01 into nushell:main Oct 25, 2023
19 checks passed
@amtoine amtoine deleted the deprecate-glob-not-to-glob-prune branch October 25, 2023 15:12
WindSoilder pushed a commit that referenced this pull request Nov 16, 2023
follow-up to
- #10827

> **Important**  
> wait for between 0.87 and 0.88 to land this

# Description
after deprecation comes removal... this PR removes `glob --not` in favor
of `glob --exclude`.

# User-Facing Changes
`glob --not` will stop working.

# Tests + Formatting

# After Submitting
i didn't find any use of `glob --not` in the `nu_scripts` so no update
required there 👍
melMass added a commit to amtoine/nu-git-manager that referenced this pull request Nov 17, 2023
related to
- nushell/nushell#10827
- nushell/nushell#10839

## description
`glob --not` has been deprecated in
nushell/nushell#10827 and will be removed in
nushell/nushell#10839 before 0.88.0 in favor of
`glob --exclude`.

this PR performs the change in `nu-git-manager`.

Co-authored-by: Mel Massadian <melmassadian@gmail.com>
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# 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.
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
follow-up to
- nushell#10827

> **Important**  
> wait for between 0.87 and 0.88 to land this

# Description
after deprecation comes removal... this PR removes `glob --not` in favor
of `glob --exclude`.

# User-Facing Changes
`glob --not` will stop working.

# Tests + Formatting

# After Submitting
i didn't find any use of `glob --not` in the `nu_scripts` so no update
required there 👍
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
follow-up to
- nushell#10827

> **Important**  
> wait for between 0.87 and 0.88 to land this

# Description
after deprecation comes removal... this PR removes `glob --not` in favor
of `glob --exclude`.

# User-Facing Changes
`glob --not` will stop working.

# Tests + Formatting

# After Submitting
i didn't find any use of `glob --not` in the `nu_scripts` so no update
required there 👍
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Related to the deprecation of commands pr:release-note-mention Addition/Improvement to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants