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

added imports on structure docs #2577

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leportella
Copy link

I was looking the docs and this example had no imports and the final loop. Since I'm new on tornado, I thought that it would be better to have this so beginners such as me wouldn't be lost. I added the imports and the app loop.

Also, since there is no really use for the db, I would suggest to take it off, to make it workable with a simple copy-and-paste. However, since I didnt't know the purpose of this, I didn't take the db off it.

@bdarnell
Copy link
Member

The db object isn't used here, but it does have a point: this example is also meant to show one way to handle database connections and similar global state in Tornado applications (and the third argument to the URL spec).

Even with these changes, the example isn't copy/pasteable because of the missing db object (also the indentation looks wrong, but that may just be github confusing me). Maybe what's needed here is two separate examples: one copy/pasteable example showing the use of capturing groups for the story_id argument, and a separate one (maybe copy/pasteable, maybe not) showing the db setup.

The blog demo is a more complete database-using example; maybe we should just point there (although it doesn't actually use the third URLSpec argument, and instead stuffs the database in the application). Maybe a better example of that third argument is RedirectHandler.

@bdarnell bdarnell added the docs label Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants