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

Use argcomplete #162

Merged

Conversation

TimothySprague
Copy link
Contributor

Closes #136

There is something that still needs addressed though. According to the argcomplete README, either global completion needs to be enabled and we need to add the string PYTHON_ARGCOMPLETE_OK to the top of the __main__.py or a user will need to add eval "$(register-python-argcomplete scuba)" to their .bashrc in order to activate argcomplete completion.

Do we want to handle this? If so, how?

@JonathonReinhart
Copy link
Owner

JonathonReinhart commented Apr 5, 2020

This looks neat.

I think we should probably remove the --list-available-options and --list-aliases options from scuba too, since they were only there for tab completion. (And ListOptsAction and tests).

@JonathonReinhart
Copy link
Owner

JonathonReinhart commented Apr 5, 2020

I think we should add PYTHON_ARGCOMPLETE_OK since it hurts nothing and allows the user to decide how they want to enable completion (global or register per-module). Then we need to present those two options (with a link to the argcomplete README) in the scuba README.

You said

we need to add the string PYTHON_ARGCOMPLETE_OK to the top of the __main__.py

However, the argcomplete docs say:

bash will look for the string PYTHON_ARGCOMPLETE_OK in the first 1024 bytes of any executable that it's running completion for

What is the "executable" in this case? when you pip install scuba, the scuba executable is generated and looks like this:

#!/usr/bin/python3
# -*- coding: utf-8 -*-
import re
import sys

from scuba.__main__ import main

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

Do you know for sure that __main__.py is where it will look?


Edit:

Okay, I see this in the docs:

Note

If you use setuptools/distribute scripts or entry_points directives to package your module, argcomplete will follow the wrapper scripts to their destination and look for PYTHON_ARGCOMPLETE_OK in the destination code.

scuba/__main__.py Outdated Show resolved Hide resolved
scuba/__main__.py Outdated Show resolved Hide resolved
@JonathonReinhart
Copy link
Owner

Another enhancement would be to add a completer to the --image option which lists all docker images.

@TimothySprague
Copy link
Contributor Author

I considered adding a completer to --image. I think it's a good idea.

I noticed we currently do a Popen or call. Any particular reason not to use the docker package to directly access the API?

@JonathonReinhart
Copy link
Owner

I noticed we currently do a Popen or call. Any particular reason not to use the docker package to directly access the API?

Simplicity, mostly.

  • Scuba started off as a simple wrapper script so there was no point in using the API.
  • Scuba supports arbitrary docker command line arguments, which doesn't scale if scuba has to map those to API calls
  • As for external dependencies, Scuba currently only requires PyYAML (important where additional dependencies means more packages to manually transfer/install)
  • I didn't really want scuba to be anywhere in the loop of the Docker API version dependency loop -- I've seen churn on projects like gitlab-runner which uses the Docker API and keeps having to bump it to maintain compatibility

@TimothySprague
Copy link
Contributor Author

Those are all sound reasons and it makes sense to me.

Just FYI though, these changes will add argcomplete as a dependency (as noted in the updated requirements.txt and setup.py)

@JonathonReinhart
Copy link
Owner

Should we make argcomplete a soft dependency? In other words, try/except ModuleNotFoundError, and conditionally call argcomplete.autocomplete(ap)? I think the completer functions could still be attached unconditionally, and since they'll never be called without argcomplete, you can use argcomplete unconditionally inside of them.

@TimothySprague
Copy link
Contributor Author

I think that strikes a good balance if we're trying to keep the hard dependencies to a minimum.

We can use the extras features for pip and requirements.txt and then update the README.md.

@TimothySprague TimothySprague changed the title Use argcomplete WIP: Use argcomplete Apr 11, 2020
Per discussions on JonathonReinhart#162

An images completer is now present to list all available Docker images
for the `--image` option.
Fixes up the aliases completer to *not* give a suggestion when an alias
has already been specified and to warn the user if no config file is
present.
Removed `get_paths_and_config()` and updated `find_config()` to return
the loaded config.
Fixes up and adds unit tests.
@TimothySprague TimothySprague changed the title WIP: Use argcomplete Use argcomplete Apr 20, 2020
TimothySprague and others added 3 commits April 20, 2020 00:01
The `--list-aliases` option has been removed
'docker images' takes a '--format' option which allows one to specify a
Go template for formatting the output. This avoids having to manually
parse the default tabular output.

References:
- https://docs.docker.com/engine/reference/commandline/images/#format-the-output
- https://golang.org/pkg/text/template/
- https://github.com/docker/cli/blob/abafad3df202/contrib/completion/bash/docker#L213-L231
@JonathonReinhart
Copy link
Owner

JonathonReinhart commented Apr 21, 2020

I tried testing this on my Debian 10 machine, by installing python3-argcomplete (1.8.1-1) via apt, and it didn't work.

The problem was that the pip/wheel-generated console script wasn't recognized by the autocompleter as a script it should follow.

This was fixed in kislyuk/argcomplete#241 which first appeared (kislyuk/argcomplete@2559a34) in v1.10.1, which is newer than what Debian has.

So note: If you're installing Scuba with pip from a wheel (the recommended method), you need argcomplete>=1.10.1.


I then tested:

  • Install argcomplete:
    • pip3 install argcomplete
    • On Debian, running pip3 install as a normal user implies --user
  • Activate for my user:
    • activate-global-python-argcomplete3 --user
    • Result: Installing bash completion script /home/jreinhart/.bash_completion.d/python-argcomplete
  • ~/.bash_completion.d/* are not sourced automatically.
    • Fix by putting this in ~/.bash_completion (source):
      for bcfile in ~/.bash_completion.d/* ; do
        [ -f "$bcfile" ] && . $bcfile
      done

Now it works!

@JonathonReinhart
Copy link
Owner

I pushed two commits that really simplify the way get_images() is implemented. I'm sorry you had to go through all of that effort... especially considering how much easier the Python API would make this.

Copy link
Owner

@JonathonReinhart JonathonReinhart left a comment

Choose a reason for hiding this comment

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

Okay I think we're really close now. Do you want to fix these things up? If not, I can.

requirements.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scuba/__main__.py Outdated Show resolved Hide resolved
scuba/__main__.py Outdated Show resolved Hide resolved
scuba/config.py Outdated Show resolved Hide resolved
scuba/config.py Outdated Show resolved Hide resolved
@TimothySprague
Copy link
Contributor Author

This was fixed in kislyuk/argcomplete#241 which first appeared (kislyuk/argcomplete@2559a34) in v1.10.1, which is newer than what Debian has.

So note: If you're installing Scuba with pip from a wheel (the recommended method), you need argcomplete>=1.10.1.

Good catch, I typically do a pip install over apt, so I wouldn't have caught this. I'll be sure to update requirements.txt.

I pushed two commits that really simplify the way get_images() is implemented. I'm sorry you had to go through all of that effort... especially considering how much easier the Python API would make this.

Nice. I'm not super familiar with all the --format values, so I tend to forget it exists (and end up doing things the hard way).

Okay I think we're really close now. Do you want to fix these things up? If not, I can.

I'm fine to make the changes.

These changes are from JonathonReinhart#162

* Fix incorrect syntax in `requirements.txt`
* Add version specifier for `argcomplete` based on a needed bug fix
* Update `README.md` example `pip install` with argument completion
support to use lowercase
* Update `README.md` to include more details for activating bash
completion
* Move the completer functions to keep the argument parser defintion
grouping and add an expected argument instead of using the `kwargs`
catch-all
* Fix a typo
* Update a misleading variable name
@JonathonReinhart
Copy link
Owner

JonathonReinhart commented Apr 24, 2020

There's one last thing that kind of stinks: argcomplete seems to always suggest positional arguments, short options, and long options.

If I have a .scuba.yml file with hello and goodbye aliases, and type scuba <TAB><TAB>, I get these suggestions:

-d            --dry-run     --entrypoint  goodbye       hello         --image       -r            --shell       -V            --version     
--docker-arg  -e            --env         -h            --help        -n            --root        -v            --verbose

This isn't horrible, but it really hurts trying to find the alias you're looking for.

This is in contrast with the way the Bash completion works for git. With git, options are only suggested if you have first typed -.

$ git checkout <TAB><TAB>
(all branches and tags are suggested)
$ git checkout -<TAB>
(nothing suggested)
$ git checkout --<TAB>
--conflict=                   --ignore-skip-worktree-bits   --no-quiet                    --patch                       --recurse-submodules          
--detach                      --merge                       --orphan=                     --progress                    --theirs                      
--ignore-other-worktrees      --no-...                      --ours                        --quiet                       --track   
$ git checkout --no-<TAB>
--no-conflict                    --no-ignore-other-worktrees      --no-merge                       --no-patch                       --no-quiet                       --no-track 
--no-detach                      --no-ignore-skip-worktree-bits   --no-orphan                      --no-progress                    --no-recurse-submodules  

I wonder if there's a way to accomplish something similar with argcomplete...


Okay I set always_complete_options=False which avoids suggesting options until the user has entered -.

By setting always_complete_options=False, options will only be suggested
if the user has enetered -
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.

Use argcomplete
2 participants