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

route app.run through the cli whenever possible #2781

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@miguelgrinberg
Member

miguelgrinberg commented May 13, 2018

This change expands the venerable app.run() function with cli support. At this point this is a proof of concept to investigate if this can be done without breaking existing applications. Feedback appreciated!

Given an application myapp.py, which ends with:

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

The following features are supported:

  • Running python myapp.py with no additional arguments is redirected to flask run in the cli module. The value of FLASK_APP is determined through introspection if not provided. If a value for FLASK_APP cannot be determined, then the original app.run() function is used.
  • Running python myapp.py with any arguments redirects to the flask command with the same arguments. For example: python myapp.py --help is the same as flask --help, python myapp.py shell starts the shell, and so on. Custom cli commands are also supported.
  • If the script passes host or port arguments in app.run(), those are also passed on to the flask run command in the cli module.
  • If the script calls app.run(debug=True) then FLASK_ENV=development is added to the environment (only if FLASK_ENV is not set).
  • If the app.run() call specifies any custom settings besides host, port, and debug, then the original logic is used for now. I can go through all the available options to run_simple() and see if there are other ones that can be supported through the cli.
return
# if the stack has two frames, it means that app.run() was called
# from the main script of the application, which is likely a
# mistake unless this is comming from an app.run() call

This comment has been minimized.

@ThiefMaster

ThiefMaster May 13, 2018

Member

s/comming/coming

@miguelgrinberg

This comment has been minimized.

Member

miguelgrinberg commented Oct 31, 2018

Guys, this change has been gathering dust since May. Anything I can do to make it more palatable and hopefully get it in?

The most important reason I want this (or something similar) in is that it brings parity between the two methods of starting a Flask app. A lot of people prefer and still use the app.run() method to start the app, but they are at a disadvantage because they are left out of custom CLI commands, and are also affected by the reloader bugs that originated the CLI in the first place. This change I believe addresses both concerns.

os.environ['FLASK_ENV'] = 'development' if debug else 'production'
# look for the flask executable in the same location where python is
executable = os.path.join(os.path.dirname(sys.executable), 'flask')

This comment has been minimized.

@davidism

davidism Oct 31, 2018

Member

I think this will fail on Windows, where it's "flask.exe".

This comment has been minimized.

@miguelgrinberg

miguelgrinberg Oct 31, 2018

Member

I'll find a Windows machine to test this, but I don't think it will fail. I don't think you have to provide the extension, the OS tries a bunch of known executable extensions such as .exe, .com and .bat until one matches.

This comment has been minimized.

@miguelgrinberg

miguelgrinberg Nov 3, 2018

Member

@davidism no problem at all running this patch on Windows.

# the call stack
from inspect import stack
called_from_main = len(stack()) == 2
# Change this into a no-op if the server is invoked from the
# command line. Have a look at cli.py for more information.

This comment has been minimized.

@davidism

davidism Oct 31, 2018

Member

Need to update this comment.

explain_ignored_app_run()
return
if load_dotenv and not options:

This comment has been minimized.

@davidism

davidism Oct 31, 2018

Member

I'm uncomfortable with this part long term. I think people are going to be surprised when suddenly their python myapp.py shell command fails because they passed another option to app.run.

args = sys.argv[1:]
sys.argv = [executable] + args
os.environ['FLASK_RUN_FROM_APP_RUN'] = 'true'

This comment has been minimized.

@davidism

davidism Oct 31, 2018

Member

Should this env var be set by app.run as well since that's what its name implies?

This comment has been minimized.

@miguelgrinberg

miguelgrinberg Oct 31, 2018

Member

Maybe it would be more clear if this var was called FLASK_RUN_CLI_FROM_APP_RUN? Because the only time you want this to be set is when the app.run call is routed through the CLI.

@@ -874,6 +874,50 @@ def routes_command(sort, all_methods):
))
def run_from_app_run(app, host=None, port=None, debug=None):

This comment has been minimized.

@davidism

davidism Oct 31, 2018

Member

This is now a third place, in addition to app.run and run_command, where a different subset of args is supported.

This comment has been minimized.

@miguelgrinberg

miguelgrinberg Oct 31, 2018

Member

Yes. It's unfortunate, but I see it as a way to converge the current two ways to start an app into a unified solution, because eventually the arguments in app.run could be deprecated in favor of CLI arguments.

@davidism

This comment has been minimized.

Member

davidism commented Oct 31, 2018

I suppose I approve this, but I don't like how circuitous and duplicated it's becoming (that's not your fault though). I'd really like to investigate how we can actually unify the code of Flask.run and run_command. Needs docs, tests, and changelog before merge.

I'm currently focused on other things (in Pallets and in general), so I won't be able to make a release for a while longer unless someone wants to handle more of the process for me (check what's been merged since the last tag, ensure changelog is up to date and links to issues/PRs, decide to merge any other PRs for a feature release, etc.).

args += ['-p', port]
else:
# pass all the original arguments directly into the cli
args = sys.argv[1:]

This comment has been minimized.

@davidism

davidism Oct 31, 2018

Member

If users were doing their own CLI parsing in __main__ then calling app.run, this will fail, because sys.argv will still contain any arguments passed to python myapp.py, that probably don't match up with Flask's CLI arguments.

For example, python myapp.py --config=dev, with app.run() in a __main__ block, called_from_main will still be true, but the args will no longer line up.

This comment has been minimized.

@miguelgrinberg

miguelgrinberg Oct 31, 2018

Member

True. There are a couple of ways to go about this. We could add yet another option to app.run to disable CLI processing. We could also add an optional args option that allows the caller to provide an alternative argument list.

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