Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Repaired get_options function in the ShowUrls command to avoid conflicts with other commands #88

Merged
merged 1 commit into from
Feb 9, 2014
Merged

Conversation

fgimian
Copy link
Contributor

@fgimian fgimian commented Feb 9, 2014

Hey there Sean, hope you're doing well 😄

I discovered a rather nasty bug which is squashed with this commit. The get_options function for the ShowUrls command was actually calling the super version of the function which was leading to conflicts with other commands.

You may reproduce the problem as follows...

Example Script 1

#!/usr/bin/env python
from flask import Flask
from flask.ext.script import Manager
from flask.ext.script.commands import ShowUrls, Clean

app = Flask(__name__)
manager = Manager(app)
manager.add_command('clean', Clean())
manager.add_command('urls', ShowUrls())

if __name__ == '__main__':
    manager.run()

Now run:

./manage1.py clean

The error output is shown below:

(flask)fots@fotsies-ubprecise-01:~/flask/flaskage$ ./manage1.py clean
Traceback (most recent call last):
  File "./manage1.py", line 12, in <module>
    manager.run()
  File "/home/fots/.virtualenv/flask/local/lib/python2.7/site-packages/flask_script/__init__.py", line 405, in run
    result = self.handle(sys.argv[0], sys.argv[1:])
  File "/home/fots/.virtualenv/flask/local/lib/python2.7/site-packages/flask_script/__init__.py", line 384, in handle
    return handle(app, *positional_args, **kwargs)
  File "/home/fots/.virtualenv/flask/local/lib/python2.7/site-packages/flask_script/commands.py", line 145, in handle
    return self.run(*args, **kwargs)
TypeError: run() got an unexpected keyword argument 'url'

As you can see, what's happening here is the run() command for the Clean command class is seeing the url option from the ShowUrls class.

Example Script 2

#!/usr/bin/env python
from flask.ext.script.commands import ShowUrls, Clean

clean_command = Clean()

print ('clean_command args before ShowUrls: %r' %
       [c.args for c in clean_command.get_options()])

urls_command = ShowUrls()

print ('urls_command args after ShowUrls: %r' %
       [c.args for c in urls_command.get_options()])
print ('clean_command args after ShowUrls: %r' %
       [c.args for c in clean_command.get_options()])

And the output:

(flask)fots@fotsies-ubprecise-01:~/flask/flaskage$ ./manage2.py
clean_command args before ShowUrls: []
urls_command args after ShowUrls: [('url',), ('--order',)]
clean_command args after ShowUrls: [('url',), ('--order',)]

This shows the problem even more clearly as the instance of the Clean command gets a different set of args after ShowUrls is imported and used.

Cheers
Fotis

@fgimian
Copy link
Contributor Author

fgimian commented Feb 9, 2014

Regarding the failed test against Python 2.6, although Travis CI doesn't represent this, the test was also failing prior to my commits.

Please see further information below:

(flask-2.6)fots@fotsies-ubprecise-01:~$ pip freeze | egrep -i script
(flask-2.6)fots@fotsies-ubprecise-01:~$ git clone git@github.com:techniq/flask-script.git
Cloning into 'flask-script'...
Enter passphrase for key '/home/fots/.ssh/id_rsa':
remote: Reusing existing pack: 1182, done.
remote: Total 1182 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (1182/1182), 333.20 KiB | 58 KiB/s, done.
Resolving deltas: 100% (566/566), done.
(flask-2.6)fots@fotsies-ubprecise-01:~$ cd flask-script/
(flask-2.6)fots@fotsies-ubprecise-01:~/flask-script$ git log --name-status HEAD^..HEAD
commit 737edbb4085fc9e97a5b31d19836674beab8ad73
Merge: a022a26 84e9568
Author: Sean Lynch <techniq35@gmail.com>
Date:   Mon Jan 27 05:23:39 2014 -0800

    Merge pull request #85 from Turbo87/submanager-help

    Show full help for submanagers if called without arguments

commit 84e9568cfa234ec1ce7ce72b5c74c4fd216bbfb9
Author: Tobias Bieniek <Tobias.Bieniek@gmx.de>
Date:   Sun Jan 26 00:02:34 2014 +0100

    Show full help for submanagers if called without arguments

M       flask_script/__init__.py
M       tests.py

commit a90431e9b486ba92c2c7db1e3428e94257519bae
Author: Tobias Bieniek <Tobias.Bieniek@gmx.de>
Date:   Sat Jan 25 22:32:53 2014 +0100

    tests: Make sure the help message is printed if no command is given

M       tests.py
(flask-2.6)fots@fotsies-ubprecise-01:~/flask-script$ tox
GLOB sdist-make: /home/fots/flask-script/setup.py
py26 create: /home/fots/flask-script/.tox/py26
py26 installdeps: -r/home/fots/flask-script/requirements.txt, pytest
py26 inst: /home/fots/flask-script/.tox/dist/Flask-Script-0.6.6.zip
py26 runtests: PYTHONHASHSEED='3300912839'
py26 runtests: commands[0] | py.test tests.py --assert=reinterp
================================================================ test session starts =================================================================
platform linux2 -- Python 2.6.7 -- py-1.4.20 -- pytest-2.5.2
collected 41 items

tests.py ..........................F..............

====================================================================== FAILURES ======================================================================
___________________________________________________________ TestManager.test_run_catch_all ___________________________________________________________

self = <tests.TestManager instance at 0x1e975a8>, capsys = <_pytest.capture.CaptureFixture instance at 0x1e97200>

    def test_run_catch_all(self, capsys):
        manager = Manager(self.app)
        manager.add_command('catch', CommandWithCatchAll())

        code = run('manage.py catch pos1 --foo pos2 --bar', lambda: manager.run())
        out, err = capsys.readouterr()
>       assert code == 0
E       assert 2 == 0

tests.py:512: AssertionError
======================================================== 1 failed, 40 passed in 0.40 seconds =========================================================
ERROR: InvocationError: '/home/fots/flask-script/.tox/py26/bin/py.test tests.py --assert=reinterp'
py27 create: /home/fots/flask-script/.tox/py27
py27 installdeps: -r/home/fots/flask-script/requirements.txt, pytest
py27 inst: /home/fots/flask-script/.tox/dist/Flask-Script-0.6.6.zip
py27 runtests: PYTHONHASHSEED='3300912839'
py27 runtests: commands[0] | py.test tests.py --assert=reinterp
================================================================ test session starts =================================================================
platform linux2 -- Python 2.7.3 -- py-1.4.20 -- pytest-2.5.2
collected 41 items

tests.py .........................................

============================================================= 41 passed in 0.34 seconds ==============================================================
py33 create: /home/fots/flask-script/.tox/py33
ERROR: InterpreterNotFound: python3.3
______________________________________________________________________ summary _______________________________________________________________________
ERROR:   py26: commands failed
  py27: commands succeeded
ERROR:   py33: InterpreterNotFound: python3.3

@techniq
Copy link
Collaborator

techniq commented Feb 9, 2014

Nice catch. It appears we were editing a shared instance when modifying the result of super(), which kind of makes sense. As for the failing test, I just ran tox locally and 2.6, 2.7, and 3.3 all passed. Going to merge this change and run again to see if I get any failures. My time to maintain this project has been near non-existent for a while so I might have to look for a new maintainer soon.

techniq added a commit that referenced this pull request Feb 9, 2014
Repaired get_options function in the ShowUrls command to avoid conflicts with other commands
@techniq techniq merged commit 9255045 into smurfix:master Feb 9, 2014
@techniq
Copy link
Collaborator

techniq commented Feb 9, 2014

I just ran tox locally with your change and all passed.

@fgimian
Copy link
Contributor Author

fgimian commented Feb 10, 2014

Thanks heaps for accepting this mate, really appreciate it :)

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

Successfully merging this pull request may close these issues.

2 participants