Use proper argparse subparsers #55

Merged
merged 5 commits into from Apr 5, 2013

Projects

None yet

2 participants

@jirikuncar

Closes #29

@techniq
Collaborator
techniq commented Apr 2, 2013

Awesome :)

I started work on this a month or so ago but got side tracked. I'll look things over in the next day or two when I get time and let you know if I find anything.

I was planning on adding argcomplete (https://pypi.python.org/pypi/argcomplete/) once this change was made, so you saved me some work :). Thanks.

@jirikuncar

I'm working on argcomplete integration right now :). I will need to remove also the initial parsing of commands in __init__.py, but it will completely break your tests. I can try to rewrite them too, but it will have to be major version bump so anybody is surprised when something breaks.

Moreover, there should be some nicer way to extend method Manager.create_parser so we can use argcomplete.autocomplete(parser, ...).

@techniq
Collaborator
techniq commented Apr 2, 2013

I noticed all the tests for this pull request are failing on Travis for Python 2.5 due to the use of __self__ instead of im_self.

Changed in version 2.6: For Python 3 forward-compatibility, im_func is also available as __func__, and im_self as __self__.

http://docs.python.org/2/reference/datamodel.html#the-standard-type-hierarchy

To be honest, I'd be content with dropping 2.5 at this point and just support 2.6+ (in fact I just committed my test changes and was using the "newer" exception handling without paying attenion, which breaks 2.5


While working on the subparser change, I too was also trying to think of the best way to handle existing commands while also cleaning things up with using argparse subparsers and thought about simply making a big version jump to signify possible backwards incompatibility (maybe 1.0.0?, as long as I felt the API would remain stable for a while).

I was also looking to simplify creating commands / other Flask-Script extensions that the original script already uses argparse and simply consume them as a subparser. For instance, I think this would greatly simplify the Flask-Assets script (https://github.com/miracle2k/flask-assets/blob/master/src/flask_assets.py)

All of this also needs to be well documented (which rewriting the docs with better organization has been on my list of todos as well, but life has gotten in the way of my contributions).

@techniq
Collaborator
techniq commented Apr 3, 2013

One thing I noticed is the pull request doesn't appear to support application-wide options (manage.add_option). I added tests to master (test_global_option_provided_before_and_after_command and test_submanager_has_options) to check for this, as well as some additional asserts under test_global_option_provided_before_and_after_command for passing the argument (-c) before and after the command (which was the request in #29)

@jirikuncar

Example of argcomplete in manage.py.

#!/bin/python
## This is just an example!

from flaskr import create_app

manager = Manager(create_app)

def main():
    try:
        import argcomplete
        _create_parser = manager.create_parser

        def create_parser(*args, **kwargs):
            parser = _create_parser(*args, **kwargs)
            argcomplete.autocomplete(parser, always_complete_options=True)
            return parser

        manager.create_parser = create_parser
    except:
        pass
    manager.run()

if __name__ == "__main__":
    main()

Then you just need to register your script using eval "$(register-python-argcomplete manage.py)".

@techniq
Collaborator
techniq commented Apr 3, 2013

Regarding argcomplete, what do you think of putting the try/except logic inside Manager.create_parser and automatically setup argcomplete if the package is installed (and maybe add an argument to Manager.__init__ to disable argcomplete if desired? For registering, we should add to the docs, and mention about global registering argcomplete (https://argcomplete.readthedocs.org/en/latest/#activating-global-completion). That would mean simply installing argcomplete and having global registered argcomplete at some point with automatically enable completion without having to do the extra steps for each application/project.

I synced up with your latest commit and noticed a few things:

  • If a command is not present (python manage.py), we should print --help, which is what we do now
  • python manage.py --help has what looks to be debug info under the description and positional arguments. Here's the output when I run python manage.py --help under the examples directory
usage: manage.py [-h] [-c CONFIG]

                 {shell,outputplus,clean,dumpconfig,runserver,urls,runservernoreload,output,getrole,optional,getrolesimple}
                 ...

positional arguments:
  {shell,outputplus,clean,dumpconfig,runserver,urls,runservernoreload,output,getrole,optional,getrolesimple}
    shell               Runs a Python shell inside Flask application context.
    outputplus          print name and url
    clean               Remove *.pyc and *.pyo files recursively starting at
                        current directory
    dumpconfig          Dumps config
    runserver           Runs the Flask development server i.e. app.run()
    urls                Displays all of the url matching routes for the
                        project
    runservernoreload   Runs the Flask development server i.e. app.run()
    output              print something
    getrole
    optional            print name and url
    getrolesimple

optional arguments:
  -h, --help            show this help message and exit
  -c CONFIG, --config CONFIG
                        config file
  • We still need to support passing global options before and after the positional arguments. For example, "python manage.py -c Development shell" and "python manage.py shell -c Development" should be valid. This could probably be solved by create a base parser and setting it as the parent for all Manager/SubManager/Command parser instances (http://docs.python.org/dev/library/argparse.html#parents).

Thanks for working on this, it's getting close. If you need me to help with anything, let me know.

@jirikuncar

Finally tests are passing and it does exactly what we need. Could you please add a mention about argcomplete to the docs?

@techniq
Collaborator
techniq commented Apr 4, 2013

Great work, thanks for this. I will merge it in tonight.

I plan to overhaul the docs before releasing the next version, and will mention about installing / using argcomplete. I also plan to bump the version, and debating on 0.7.0 or straight to 1.0.0.

I'm actually having trouble setting up argcomplete myself (I upgraded bash from the Mac default 3.2 to 4.2 via brew, which is needed for global completion). I also added...

#!/usr/bin/env python
# PYTHON_ARGCOMPLETE_OK
...

...to the top of the examples/manage.py and also chmod +x manage.py but nothing seems to complete. I'm sure it's a system setup issue and will look at it more tonight.


I have a couple questions regarding the pull request (hopefully my last :) )

I noticed your setting a global variable ARG_COMLETE at the top instead of just doing this try/except around 168-172. Any reason behind this (performance, etc), as otherwise I'll probably make that change (there was also a typo in the variable name ;) ).

Also, I still see all the command names being printed under usage and position arguments (see my comment a few back). Any idea on what is causing this?

Lastly, I think there is a left over _handle method on Manager during your iterations that I think I can remove. Am I right? I also plan to remove get_usage and print_usage since we now rely on argparse for this, and can't think of a reason why someone would have overridden these themselves.

@jirikuncar

I fixed the typo and made a cleaning of unused methods on Manager.


I'm using bash from Brew and argcomplete from PyPI.

> echo $BASH_VERSION
4.2.42(2)-release
> pip freeze | grep argcomplete
argcomplete==0.4.0

I have added following line (which register-python-argcomplete >/dev/null) && eval "$(register-python-argcomplete 'manage')" to ~/.virtualenvs/<name>/bin/activate for registering my manage script.

@techniq
Collaborator
techniq commented Apr 5, 2013

I look to have everything in order.

$ echo $BASH_VERSION
4.2.45(2)-release

$ pip freeze | grep argcomplete
argcomplete==0.4.0

$ which register-python-argcomplete 
/Users/smlynch/.virtualenvs/flask-script/bin/register-python-argcomplete

$ eval "$(register-python-argcomplete 'manage')"

I also have the global setup using ~/.bash_completion.d/python-argcomplete.sh and put the # PYTHON_ARGCOMPLETE_OK as the second line within manage.py

Tabbing just shows files (I also tried python manage.py <TAB>, and tab completing commands (s for shell) or arguments (--h)

$ ./manage.py 
manage.py   manage.pyc  prod.cfg    

I'm going to go ahead and merge this as it looks good. I might consider changing the completer when bpython or ipython is installed, but I'm going to first get it working normally and play with it before doing so (https://argcomplete.readthedocs.org/en/latest/#readline-style-completers).

Once again, thanks for this.

@techniq techniq merged commit 8fdc0c6 into smurfix:master Apr 5, 2013

1 check passed

Details default The Travis build passed
@techniq
Collaborator
techniq commented Apr 9, 2013

Finally figured out why I couldn't get tab completion working... I never sourced ~/.bash_completion.d/python-argcomplete.sh in my ~/.bash_profile (I was thinking anything in ~/.bash_completion.d/ was auto-sourced until I saw I had sourced the brew completer.

Have you played around with changing out the readline completor and using the ipython or bpython one? I'm considering using one of them if either is installed (similar to how the shell command does - https://github.com/techniq/flask-script/blob/master/flask_script/commands.py#L242).

@jirikuncar

Good news :) I knew that it must be something trivial.

I was thinking anything in ~/.bash_completion.d/ was auto-sourced

Maybe adding something like for c in ~/.bash_completion.d/* ; do . $c; done to ~/.bash_profile could help? (probably not the best solution ...)

Have you played around with changing out the readline completor and using the ipython or bpython one?

No, I haven't tried it yet.

@techniq
Collaborator
techniq commented Jun 13, 2013

Would you by chance have time to look at the python3 branch? There is 1 test failing only on 3.3 regarding submanagers I haven't been able to figure out and thought a fresh pair of eyes might help :)

I setup tox so you can run it to see 2.6/2.7 pass and 3.3 fail the 1 test.

@jirikuncar

I have troubles with Flask-0.10.1 on Python 3.3.0.

AttributeError: 'AssertionRewritingHook' object has no attribute 'is_package'

There are my packages pip freeze:

Flask==0.10.1
-e git+https://github.com/techniq/flask-script.git@6e47afcea25e2ad26a9c32371b3ebe34f3704b12#egg=Flask_Script-dev
Jinja2==2.7
MarkupSafe==0.18
Werkzeug==0.9.1
distribute==0.6.34
itsdangerous==0.21
py==1.4.14
pyflakes==0.7.2
pytest==2.3.5

Do you have any suggestion how to solve it? I have to admin, that I've never run Python 3 code before :(

@techniq
Collaborator
techniq commented Jun 14, 2013

Yes, it is a known issue with pytest and Flask (I reported it :) ).

The workaround is to not to use the assertion statement rewriting but switch to either plain (--assert=plain) or reinterpretation (--assert=reinterp) assertion introspection, the later being what I use (and is set in the tox.ini file).

I'm assuming your not running the tests on the console (if you are I would recommend using tox) but are probably using something like PyCharm. If you are using PyCharm, you can edit the test confirmation, check the Options checkbox, and set the value to --assert=reinterp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment