0.5.3 to 0.6.x #71

Closed
locojay opened this Issue Aug 13, 2013 · 14 comments

7 participants

@locojay

after upgrading to 0.6.x i get

Traceback (most recent call last):
  File "manage.py", line 119, in <module>
    manager.run()
  File "/Users/locojay/.virtualenvs/hamss/lib/python2.7/site-packages/flask_script/__init__.py", line 366, in run
    raise e
TypeError: create_parser() got an unexpected keyword argument 'parents'
@techniq
Collaborator

Do you have any custom commands?

@locojay

seems to be a problem with flask-assets

manager.add_command("assets", ManageAssets(assets))

commenting out works

@techniq
Collaborator

I was concerned this might happen when we switched to using argparse's subparsers as the signature of that method changed (fixes issue #29, and allows for argcomplete support / etc).

The fix will probably need to come from Flask-Assets, by adding changing def create_parser(self, prog): to def create_parser(self, prog, parents=None):
https://github.com/miracle2k/flask-assets/blob/master/src/flask_assets.py#L393

One thing I need to think more about is for extensions like this that consume all arguments, they will still ignore parents which are the Flask-Script options, or "global arguments" for things like configuration loading (http://flask-script.readthedocs.org/en/latest/#adding-options-to-the-manager). This would mean you couldn't do something like python manage.py assests build -c Development without consuming the parents when constructing the argparse.ArgumentParser() within webassets.

To support these changes it looks like changes would need to be made in both Flask-Assets and WebAssets around:
https://github.com/miracle2k/flask-assets/blob/master/src/flask_assets.py#L342
https://github.com/miracle2k/webassets/blob/master/src/webassets/script.py#L430

@techniq
Collaborator

Thinking about this some more, it might be best if we change our signature for create_parser to take in kwargs, this would future proof us some more in case we pass more and not require all the custom commands to update their signatures (as long as they consume kwargs and pass them along)

@techniq techniq referenced this issue in miracle2k/flask-assets Aug 26, 2013
Closed

Support for Flask-Script 0.6 #52

@nibrahim

FWIW, I get the same problem because of some custom command for alembic (the db migration tool).

@iurisilvio

Same here. Flask-Alembic is broken.

@techniq
Collaborator

I spent a good bit of time tonight trying to port Flask-Assets and Flask-Alembic to be compatible with Flask-Script 0.6.x (which uses Argparse's subparsers).

Most of my time was spent looking at Flask-Assets, which ultimately has a major chicken/egg problem. The extension doesn't build it's ArgumentParser instance until the .run() call, instead of in create_parser() because it uses Flask's current_app config as one way to load the Jinja env (it can also be passed explicitly in the ManageAssets command's constructor). I've been trying to find a way to refactor Flask-Script / Flask-Asset / or webassets so I can create the parser without an app context, or get an app context created before creating the parser, but that's another chicken/egg situation (an argument from the parser may be used to influence how the app is created, for example passing "-c Development" to load a development config). At this point I'm not sure how best to proceed on Flask-Assets, and ultimately a hack may be needed to get partial support.

@iurisilvio Regarding Flask-Alembic, it looks like I can fix it by doing something like:

from alembic.config import CommandLine
...

class ManageMigrations(Command):
...
    def create_parser(self, *args, **kwargs)
        return CommandLine(kwargs['prog']).parser

https://github.com/tobiasandtobias/flask-alembic/blob/master/flask_alembic/__init__.py#L32

One issue with Flask-Alembic is it appears to be unmaintained/abandoned and is not listed on PyPI. I saw another extension, Flask-Migrate, which does look to be maintained and listed on PyPI, and most importantly, just from looking at the source, should be compatible with Flask-Script 0.6.x+. Maybe give that extension a try first before I update Flask-Alembic which probably will end up being a lost pull request (you could always install from my fork if I do make the change, but really don't plan to maintain it if that happens).

On a side note, I've been running into problems finding time to maintain Flask-Script and may have to look for another maintainer (at least co-maintainer) if my lack of available time keeps up.

@iurisilvio

Yes, I found Flask-Migrate and will probably change to it. It is a really new project (added to GitHub 12 days ago), but it looks great.

Thanks!

@miguelgrinberg

Sean, the following patch, combined with a minor change in Flask-Assets seems to do the trick.

diff --git a/flask_script/__init__.py b/flask_script/__init__.py
index 9edbf18..940c3a4 100644
--- a/flask_script/__init__.py
+++ b/flask_script/__init__.py
@@ -155,9 +155,13 @@ class Manager(object):

         for name, command in self._commands.items():
             description = getattr(command, 'description', command.__doc__)
-            command_parser = command.create_parser(name, parents=[options_parser])
+            try:
+                command_parser = command.create_parser(name, parents=[options_parser])
+            except TypeError:
+                command_parser = command.create_parser(name)
             subparser = subparsers.add_parser(name, usage=description, help=description,
                                               parents=[command_parser], add_help=False)
+            subparser.set_defaults(func_handle=command.handle)


         ## enable autocomplete only for parent parser when argcomplete is

And the change in Flask-Assets is:

diff --git a/src/flask_assets.py b/src/flask_assets.py
index 038cb3e..c8c7b2b
--- a/src/flask_assets.py
+++ b/src/flask_assets.py
@@ -330,7 +330,7 @@ else:
     import argparse
     from webassets.script import GenericArgparseImplementation, CommandError
-    class CatchAllParser(object):
+    class CatchAllParser(argparse.ArgumentParser):
         def parse_known_args(self, app_args):
             return argparse.Namespace(), app_args

Basically on the Flask-Script side this change allows the two formats of create_parser. First the new format with the parents argument is tried, if that doesn't work the old style is used, and in this case the subparser will not offer the options of the parent, of course.

On the Flask-Assets side the only problem is that the CatchAllParser object was not a subclass of ArgumentParser, so it could not be used itself as a parent of another parser. Making it a subclass seems to address that.

Let me know if this looks good. I'll send you the pull request.

Miguel

@techniq
Collaborator

This looks reasonable and would fix the current issue, but here's some issues I see

  • If Flask-Assets (or any extension) does not take in and consume the parent attribute, this means all options from the Manager instance will be ignored when using that command. For example, if you have a config loading option python manage.py -c Development assets build, it would be ignored and whatever your default config is would be loaded.
  • I'm hoping to have extensions no longer need/using a CatchAllParser and thus accessing sys.argv but instead fully rely on argparse. One of the big features for going full argparse is to allow us to specific options in any order. For example python manage.py -c Development some_command or python manage.py some_command -c Development. Also, this breaks the extension from being used as part of a SubManager as it always expects to be at argv[1]

This patch may be enough of a band aid to buy more time for a better fix though.

@miguelgrinberg

The way I see it is that my change gives new Command subclasses the full subparser support, while still allowing the older pre 0.6 classes to continue to work. I guess you could show a warning indicating that the Command needs to be updated to take advantage of all the features when you get an old style Command registered if you feel it is important to get people on the latest stuff.

Regarding your first bullet above, it seems when you do manage.py -c CONFIG some_command the -c is recognized just fine for a parser that doesn't deal with parents. The -c is ignored only when it appears after the command name. I'm pretty sure I saw it work this way yesterday, so it isn't as bad as you make it sound. Am I mistaken?

Your second point is a good goal, but you can leave the decision to the developer of each extension. You provide the framework that enables the greater flexibility, but you still tolerate the older clients that don't know or care about that.

As a side note, when I was looking for a solution, I was hoping argparse would have a way to hook up a parent to an existing ArgumentParser instance. That would have been the perfect solution, but the helper function that moves the parent options to the child is private.

@samyakbhuta samyakbhuta referenced this issue in tobiasandtobias/flask-alembic Oct 10, 2013
Open

Support for Flask-Script 0.6 #8

@yalon yalon added a commit that referenced this issue Nov 10, 2013
@yalon yalon Workaround for #71
check if create_parser() supports parents= parameter or not.
8e31a3e
@mkoryak

+1 Could really use a fix here ;)

@techniq
Collaborator

I just pushed 0.6.4 to PyPI which includes the workaround suggested by @miguelgrinberg. I currently raise a UserWarning if parents is not one of a command's create_parser arguments. I didn't raise it as a DeprecationWarning so it wouldn't be ignored by default as I hope third-party commands update to use argparse and take in the parents argument.

Note, if a third party command doesn't take in / use the parents argument, I don't think global options (such as passing in a -c config flag) will be honored (see my first bullet point on this comment.

@jeffwidman
Collaborator

@techniq can this issue be closed?

@jeffwidman jeffwidman closed this Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment