Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

support for flask app factory pattern #85

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

frankV
Copy link

@frankV frankV commented Jul 3, 2016

settled the conflicts I found after fetching #55 @andviro

closes #81

@frankV
Copy link
Author

frankV commented Jul 7, 2016

ping ;)

The errors from CircleCI are just pyflakes. There's quite a few for * imports. Do you guys want to update the flake8 config or fix them?

'''
Takes optional Flask application instance. If supplied, `init_app` will be
called to update application url map.

Copy link
Member

@wojcikstefan wojcikstefan Jul 9, 2016

Choose a reason for hiding this comment

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

unnecessary newline

@wojcikstefan
Copy link
Member

Hey @frankV, thanks for championing this :) Overall it looks good aside from a few minor issues I commented on in the code (though @thomasst should also take a look). As for pyflakes, I see that all of the issues happen in the example code. We should probably move to named imports in there instead of ignoring more rules. That said, the errors only manifested in this PR because its build was triggered with an upgraded flake8 version. I'm fine with opening a separate issue for that and ignoring F405 here.

@frankV
Copy link
Author

frankV commented Jul 9, 2016

@wojcikstefan fixed up the minor things and I'll finish up the others tomorrow. I'll wait for @thomasst's remarks before issuing another PR.

@frankV
Copy link
Author

frankV commented Aug 24, 2016

didn't forget about this, getting back to it so for now I'm going to reissue the PR with the changes I implemented... let's go from there

@frankV
Copy link
Author

frankV commented Aug 24, 2016

@wojcikstefan I got it all in with the exception of

  1. didn't skip the "ugly hasattr conditions" in init but I can with a little more time
  2. updated the formatting to my example solution on init line 87

@frankV
Copy link
Author

frankV commented Sep 4, 2016

Just wanted to ping you guys one more time on this, let me know if you want something changed.

Let's get this merged in! 👍

@wojcikstefan
Copy link
Member

Hi @frankV, I've been on vacation for the last 2 weeks - will look at it soon! Thank you so much for following up on this one!

@thomasst
Copy link
Member

I just reviewed this code and read up on init_app.

While this PR allows lazy initialization, it still doesn't conform to the guidelines. See http://flask.pocoo.org/docs/0.11/extensiondev/:

Note on init_app
As you noticed, init_app does not assign app to self. This is intentional! Class based Flask extensions must only store the application on the object when the application was passed to the constructor. This tells the extension: I am not interested in using multiple applications.

When the extension needs to find the current application and it does not have a reference to it, it must either use the current_app context local or change the API in a way that you can pass the application explicitly.

That being said, I'm not sure if there's a good solution to make sure the URL map is applied to all the apps, in case init_app is called multiple times with different apps. Thoughts?

@frankV
Copy link
Author

frankV commented Sep 29, 2016

I'm not sure I can understand the use case, but that may be my limited experience with using multiple apps.

All I can think to do is investigate how similar extensions handle this, then test it out in my fork.

@sriramsv
Copy link

Hey, can someone please merge this pull request. I need this functionality in one of my projects

@richin13
Copy link

richin13 commented Aug 8, 2018

Lazy initialization is a must on any flask extension, will this ever be merged? 😢

@adnanmuttaleb
Copy link

Ohh god 3 years and init_app still not added, why!!

@AlecRosenbaum
Copy link
Contributor

Sorry! I just stumbled across this and didn't realize a PR was pending here for so long. I've just merged delayed init_app support with #137

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.

Support lazy initialization / use init_app
10 participants