feat(new): Parse args, support Pyenv & much more

[justinmayer]

This adds argument parsing to the vf new command (#71) via Fish's argparse function, which I then subsequently used to provide the following enhancements:

• Find & use non-PATH Python interpreters via common paths/tools (Pyenv, Pythonz, etc.) (#114)
• Virtual environment name no longer needs to be passed as last argument (#16)
• Control output verbosity via --quiet, --verbose, & --debug flags
• Add help via vf new --help option flag (#146)

[justinmayer]

This takes what was a relative simple function and makes it significantly more complex. I imagine there are more elegant ways to implement some of this functionality, and I'm open to specific suggestions as to how it might be improved. At the same time, I'm excited about both the magnitude and quantity of the enhancements within, and I have no problem with making it work first and then making it more elegant later.

[justinmayer]

Hi @zanchey, @faho, @terlar, and @krader1961. Please accept my sincere apologies in advance for the intrusion. Before merging this sizable chunk of functionality into VirtualFish, I could really use an extra pair of eyes on this pull request. I respect your extensive Fish shell knowledge and experience, and I would be most grateful for any suggestions for improvement you might have. 🙇

Once again, I'm sorry if I'm adding noise to your undoubtedly already-overflowing inbox. If this request is unwelcome, please let me know so I can be sure to avoid bothering you in the future! 😅

[faho]

You might want to document the minimum supported fish version. argparse --ignore-unknown puts it at 3.1. argparse itself would require 2.7.

[justinmayer]

A million thanks, @faho, for the quick response and the insightful feedback. You definitely taught me a few things I did not know about. Much, much appreciated!

I made nearly all the changes you suggested, as can now be seen in this PR. I didn’t fully understand the last two comments you made about the for-loop, which actually seems to behave precisely as expected, even when given complicated invocations such as the following:

➤ vf new --system-site-packages foo -d -p 3.8.2 --clear --clear --extra-search-dir /path/to/wheels --no-wheel --pip 19.3.1 --activators fish,python
Creating foo via ~/.asdf/installs/python/3.8.2/bin/python …
Virtualenv args: --python ~/.asdf/installs/python/3.8.2/bin/python --system-site-packages --clear --clear --extra-search-dir /path/to/wheels --no-wheel --pip 19.3.1 --activators fish,python
Extra args: foo

When I tried to change the for-loop based on your feedback, however…

# Unpack Virtualenv args: first flags that need values, then Boolean flags
# When done, $argv should only contain the new virtual environment name.
set -l flags_with_args --app-data --discovery --creator --seeder --activators --extra-search-dir --pip --setuptools --wheel --prompt
for i in (seq (count $argv))
    # If arg starts with a hyphen…
    if string match -q -- "-*" $argv[$i]
        # If this option requires a value that we expect to come after it…
        if contains -- $argv[$i] $flags_with_args
            # Get that value's index number
            set -l next_arg_idx (math $i + 1)
            # Move both the option flag and its value to a separate list
            set virtualenv_args $virtualenv_args $argv[$i] $argv[$next_arg_idx]
            set -e argv[$next_arg_idx]
            set -e argv[$i]
        else
            # This option is a Boolean w/o a value. Move to separate list.
            set virtualenv_args $virtualenv_args $argv[$i]
            set -e argv[$i]
        end
    end
end

… the same invocation results in incorrect argument parsing:

Too many arguments. Except for option flags, only virtual environment name is expected:
Virtualenv args: --system-site-packages --clear --extra-search-dir /Users/jm/Virtualenvs/foxdot --pip 19.3.1
Extra args: foo --clear --no-wheel --activators fish,python

I highly suspect that is due to (1) my misunderstanding of your comments, (2) code errors in my attempt to modify the for-loop, or both.

Given all of the above, do you have any suggestions as to how I should proceed? (Once again, thank you SO much for the pointers!)

[faho]

I highly suspect that is due to (1) my misunderstanding of your comments, (2) code errors in my attempt to modify the for-loop, or both.

Or (3) a bad habit of mine. I've looked at the first point, made a comment about that, saw the second, made a comment about that, and forgot to put them together.

So... when you have a for-loop like

for fish in $aquarium

The range you loop over is effectively immutable from the inside, because, while you can modify the variable, it's already been expanded when the loop was first entered. So if you set aquarium here, it will have no effect on any of the values of $fish.

So you cannot cause the loop to skip a value by doing set -e aquarium[$index]. But that's just what you are trying with set -e argv[$next_arg_idx]. It doesn't work, because you need another kind of loop.

Now, I suggested a for i in (seq (count)) loop earlier because you tried accessing the index, but that doesn't work anymore once you also want to erase parts of the variable - because after you have done that, the index is off.

So what you need is my other suggestion, a while set -q var[1] loop:

while set -q aquarium[1]
    # We always use the first element, because we'll delete it later
    set -l fish $aquarium[1]
    if test "$fish" = "flounder"
        # This also means that we can delete the *next* element which is always element 2.
        set -e aquarium[2]
    end
    # Because we've forgone the comfort of the for-loop, we need to handle jumping to the next var ourselves
    set -e aquarium[1]

Or you can set a "skip_next" flag that you read in your for-loop, that tells you to skip the entry:

set -l skip_next 0
for fish in aquarium
    if test $skip_next = 1
        set skip_next 0
        continue
    end
    # ...
    if something
        set skip_next 1
    end
end

[justinmayer]

Ah, okay! Now I understand the pitfalls inherent in my for-loop implementation. I just pushed another commit that replaces it with a while loop, which also seems to correctly parse the example invocation mentioned above. In addition to being a more-correct solution, I get the impression it is also a bit faster.

Once again, thank you so much for taking the time to guide me to a better implementation of these new features. Truly much appreciated! 🙏🏻

[justinmayer]

Once again, thank you, Fabian. You took a significant amount out of your day to help improve this code as well as my Fish shell knowledge. Deeply appreciated. 🙇