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

Add install arguments to not set up {,f}path #271

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rcloran
Copy link
Contributor

@rcloran rcloran commented Oct 10, 2023

Before submitting your PR (pull request), please
check the following:

  • There is no other PR (open or closed)
    similar to yours. If there is, please first
    discuss over there.
  • Each commit messages follows the Seven
    Rules of a Great Commit
    Message
    .
  • Each commit message includes
    Fixes #<bug> or Resolves #<issue> in its
    body (not subject) for each issue it resolves
    (if any).
  • You have
    squashed
    any redundant or insignificant commits.

`opts_pre` was re-declared empty between first definition and use in a
second check, so was a no-op in that second check.

The parsing of `opts_post` would stop at the first "non-help" option,
meaning a `--help` or `-h` specified further down the command-line would
not be seen.

Before:

```
russell@zip:~ ❯ znap pull --foo --help
russell@zip:~ ❯ znap pull --help --foo
Usage: znap pull [ <repo> ... ]
Update repos in parallel.
russell@zip:~ ❯ znap pull bla bla --help --foo
russell@zip:~ ❯ znap pull bla bla --foo
russell@zip:~ ❯ znap pull
znap no upstream
znap-shallow ahead 3
```

After:

```
russell@zip:~/src/zsh-snap ❯ znap pull --foo --help
Usage: znap pull [ <repo> ... ]
Update repos in parallel.
russell@zip:~/src/zsh-snap ❯ znap pull --help --foo
Usage: znap pull [ <repo> ... ]
Update repos in parallel.
russell@zip:~/src/zsh-snap ❯ znap pull bla bla --help --foo
Usage: znap pull [ <repo> ... ]
Update repos in parallel.
russell@zip:~/src/zsh-snap ❯ znap pull bla bla --foo
russell@zip:~/src/zsh-snap ❯ znap pull
znap no upstream
znap-shallow ahead 3
```
Fixes marlonrichert#205

Test:

```russell@zip:~ ❯ cat test.zsh
t() {
    path=(/usr/bin /bin)
    fpath=()
    echo --
    echo znap install "$@"
    znap install "$@"
    echo path = $#path , fpath = $#fpath
}

t asdf-vm/asdf --no-path --no-fpath
t --no-path --no-fpath asdf-vm/asdf
t --no-path asdf-vm/asdf
t asdf-vm/asdf --no-path
t --no-fpath asdf-vm/asdf
t asdf-vm/asdf --no-fpath
t asdf-vm/asdf
t asdf-vm/asdf --no-such-arg
russell@zip:~ ❯ source test.zsh
--
znap install asdf-vm/asdf --no-path --no-fpath
path = 2 , fpath = 0
--
znap install --no-path --no-fpath asdf-vm/asdf
path = 2 , fpath = 0
--
znap install --no-path asdf-vm/asdf
path = 2 , fpath = 1
--
znap install asdf-vm/asdf --no-path
path = 2 , fpath = 1
--
znap install --no-fpath asdf-vm/asdf
path = 3 , fpath = 0
--
znap install asdf-vm/asdf --no-fpath
path = 3 , fpath = 0
--
znap install asdf-vm/asdf
path = 3 , fpath = 1
--
znap install asdf-vm/asdf --no-such-arg
znap clone: invalid argument '--no-such-arg'
Usage: znap clone ( <user>/<repo> | <url> ) ...
Download repos in parallel.
path = 2 , fpath = 0
```
@rcloran
Copy link
Contributor Author

rcloran commented Oct 10, 2023

It seems like since #205 was created, the behaviour of znap changed to set up PATH instead of install in ~/.local/bin. So I've chose to name the argument "--no-path" instead of the "--no-executable" suggested in that feature request. Feedback is welcome.

I haven't updated any documentation, because it seems like there wasn't an obvious place in the README. If you have suggestions on where it should go, I'm also happy to add that.

@marlonrichert
Copy link
Owner

marlonrichert commented Dec 14, 2023

Instead of putting tests in commit messages, could you rather make an actual test for this that can be run with clitest? You can use this test in zsh-autocomplete as a template. You can also copy the script for running the test manually and config for running it in GitHub CI from there.

_arguments : '*:remote repository:_urls'
_arguments : \
"--no-fpath[don't update fpath with completions or functions]" \
"--no-path[don't update PATH with binaries]" \
Copy link
Owner

Choose a reason for hiding this comment

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

So, znap --no-fpath --no-path <url> would be basically be a no-op?

@marlonrichert
Copy link
Owner

Instead of explaining before/after, could you describe scenarios where you need this? I'm not sure what problem you are trying to solve.

@marlonrichert
Copy link
Owner

Oh, and thank for submitting so many pull requests! I really appreciate it, since I haven't had much time to devote to this project later. I might even add you as a maintainer. I like your code. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants