Skip to content
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

Merged
merged 5 commits into from Jan 15, 2017
Merged

Add larger app ex #2131

merged 5 commits into from Jan 15, 2017

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Dec 30, 2016

I chose the wrong base: #2130

Original PR message:

Addresses: #1902 (comment)

Confirms that the larger application as outlined in the docs, works as expected. For the sake of less confusion, for those working through this example, having src might be helpful. Not everyone will want to work along with the docs. If there are parts of the docs that are not clear, we should fix that as well. (They seemed clear to me).

Thiefmaster's response:

The weird commits in this PR aside (rebase to upstream/master and force-push to fix), I'm not sure how useful this is to have in the repo... Especially since having a "large" app without an app factory is somewhat questionable...

My response:

@ThiefMaster Sorry about that. I didn't choose correct base. I was thinking that in order to debug issues that people are having with code examples in the docs, it would be useful to have those examples on-hand.

I agree. But maybe until we can put something better together for #2027, this addition could just be temporary and we could deprecate this "larger applications" example at later time.

@untitaker
Copy link
Contributor

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.

@ThiefMaster
Copy link
Member

I think it works since the views import is at the bottom, after app has been defined. Still a very good point though.

@untitaker
Copy link
Contributor

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 examples/ for more "complete" applications, not just patterns).

@wgwz
Copy link
Contributor Author

wgwz commented Dec 30, 2016

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?

@untitaker
Copy link
Contributor

untitaker commented Dec 30, 2016

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 tox.ini.

@kblomqvist
Copy link
Contributor

A week ago I got confused by circular imports when adding models.py to flaskr example that I was modifying to work as a large application. I think that the large app example should also showcase how to use models.

@untitaker
Copy link
Contributor

Would like to see a rewrite of flaskr with flask-sqlalchemy

@wgwz
Copy link
Contributor Author

wgwz commented Dec 30, 2016

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.

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 tox.ini.

@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.)

commands =
    # We need to install those after Flask is installed.
    pip install -e examples/flaskr
    pip install -e examples/minitwit
    pip install -e examples/patterns/largerapp
    py.test --cov=flask --cov-report html []

@untitaker
Copy link
Contributor

@wgwz Yes exactly, it's just about installing the application in the virtualenv (otherwise you can't import it in the tests)

@wgwz
Copy link
Contributor Author

wgwz commented Dec 31, 2016

@untitaker OK I understand now. Should I add some basic tests for it under tests/test_apps?

@untitaker
Copy link
Contributor

untitaker commented Dec 31, 2016 via email

- also adds this pattern into tox for testing
- moved the link towards the top for better visibility
@wgwz
Copy link
Contributor Author

wgwz commented Jan 15, 2017

should this be closed?

@untitaker untitaker merged commit 47e8410 into pallets:master Jan 15, 2017
@untitaker
Copy link
Contributor

I think it's fine as is, thanks :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
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.

None yet

4 participants