-
-
Notifications
You must be signed in to change notification settings - Fork 16.3k
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
Add larger app ex #2131
Add larger app ex #2131
Conversation
This example doesn't work because it contains a circular import and precisely shows why you need app factories. To separate views from the main app you should use blueprints. |
I think it works since the |
Ah, I overread that this is from the docs, and yes, that actually works. I don't think we need to test this in any way though (I'd reserve |
I'm not by any means saying this is best practice. Just that having the hardcoded example accompanying the docs, might lead to less confusion for those reading it. (And a sanity check that what's in the docs works as it should.) @untitaker how would you feel about adding a patterns directory under examples? |
Fair enough though this doesn't test anything because pytest doesn't import the package. We should add a test stub as well that just imports and install the package in |
A week ago I got confused by circular imports when adding |
Would like to see a rewrite of flaskr with flask-sqlalchemy |
While this PR isn't for flaskr and neither was the comment that prompted me to work on this, I do agree. flaskr could be overhauled to show current best practices; i.e. app factories, blueprints, flask-sqlalchemy and the CLI, which also helps with #2027. Maybe at that point this whole larger app portion of the docs can be written with respect to flaskr.
@untitaker assuming you want to go forward with this, would tox.ini change as below? and what else needs to change/be added? I don't follow what you are saying. (not familiar with tox.)
|
@wgwz Yes exactly, it's just about installing the application in the virtualenv (otherwise you can't import it in the tests) |
@untitaker OK I understand now. Should I add some basic tests for it under |
The other example apps had their own testfolders iirc
…On 31 December 2016 17:53:14 CET, Kyle Lawlor ***@***.***> wrote:
@untitaker OK I understand now. Should I add some basic tests for it
under `tests/test_apps`?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#2131 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
- also adds this pattern into tox for testing
- moved the link towards the top for better visibility
should this be closed? |
I think it's fine as is, thanks :) |
I chose the wrong base: #2130
Original PR message:
Thiefmaster's response:
My response: