Opened 12 years ago
Closed 12 years ago
#18715 closed Cleanup/optimization (fixed)
Tutorial Page 3 Refactor
Reported by: | Daniel Greenfeld | Owned by: | Daniel Greenfeld |
---|---|---|---|
Component: | Documentation | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | timograham@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
- URL definitions
- Text cleanup
- Implementation of Class based views
Attachments (3)
Change History (20)
comment:1 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:3 by , 12 years ago
comment:5 by , 12 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:6 by , 12 years ago
Owner: | changed from | to
---|
comment:7 by , 12 years ago
I'm not comfortable with the very verbose way of defining urls. Is this the canonical style we intend to promote? It's fine to explain the url()
function in detail but in my opinion we should quickly show the more concise and common syntax, especially since it appears several times further down the document.
Some sentences reference examples further down the document (I had written down the list but I managed to lost it, sorry -- me or someone else will have to proof-read the whole document again).
Otherwise this looks very good to me.
I've improved the markup and added some information (positional vs. keyword arguments, 500.html template, etc. -- I also lost that list). I'm attaching a consolidated patch that reflect the state after my edits.
by , 12 years ago
Attachment: | 18715.patch added |
---|
comment:9 by , 12 years ago
Cc: | added |
---|
Agreed with Aymeric's thoughts on URLs. Although I can see how it leads naturally into the detailed sections that follow, I've never used or seen such a verbose syntax. Maybe introduce that once and then explain that we don't need to include the keywords and change it to the more compact: url(r'^$', views.index, name='index')
The note "What is a URLconf?" seems a bit out of place following 4 paragraphs of detailed explanations. Move it higher and not necessarily in an admonition box?
I'm unfamiliar with absolute_import, but it seems out of place with the rest of the tutorial. May be better to convert the entire thing to use it either now or as a separate ticket?
The patch leaves the old "Removing hardcoded URLs in templates" at the very bottom of the file.
comment:11 by , 12 years ago
Would you like to make some tweaks based on my comments (or let me know if you disagree with them)?
comment:12 by , 12 years ago
I made edits based on my suggestions:
-Omitted regex and view kwargs when defining URLs
-Moved "What is a URLconf?" higher up
-Removed use of absolute_import (let's make that a separate ticket and convert the entire tutorial if it's something we want to do)
-Modified bits of tutorial 4 that are affected by this (took a quick shot at it; needs to be double checked and tested).
I plan to give this another look tomorrow and post the built HTML so we can ask people to give it a try before committing.
Thank-you for the great work Danny -- the old tutorial really does seem awkward in some spots after running through this version!
comment:13 by , 12 years ago
Here is a link to the updated tutorial 3 & 4 if you would like to review without downloading the patch and building the docs yourself.
http://techytim.com/django/tutorial03/intro/tutorial03.html
comment:14 by , 12 years ago
This is looking really good. A couple of fairly small suggestions:
- In the discussion about regexes, it says "These are super fast", and directs people to the docs on regexes. This gives me two concerns.
1) it suggests that you need to know all about regexes, when in practice, you really only need to know how to capture simple patterns.
2) it implies that it's a good idea to use the full capabilities of regexes -- which isn't correct, given that some regexes can have pathologial lookup performance.
It might be worth a qualifier to say that we're only going to touch the tip of the iceberg of what regexes can do, and that you probably shouldn't rely on the full power of regexes.
- When it introduces templates, it suggests creating a directory "anywhere on the filesystem" and putting it in TEMPLATE_DIRS, "just like we did in tutorial 2". It seems to me we're missing an opportunity to say two things:
1) That you can use the same TEMPLATE_DIRS setting from tutorial 2; you don't need to create a completely new one for your "app" templates
2) That another option is to put a templates directory in your polls app, which will be automatically discovered by Django.
My concern here is mostly that "put it anywhere" will lead to people putting it genuinely anywhere. Part of the role of the tutorial is to establish best practices, so "in the project directory" is good advice to be giving here. Plus, if we leave people to their own devices, they're going to find some interesting locations -- and some places (like the HTTP server doc root) are potentially a bad idea.
- 404.html and 500.html suggest that you should create them, but doesn't (explicitly) say you should put content in them. If I was going to guess how the tutorial would fail, it will be because "I created an empty 500.html, and now my browser is showing nothing". Suggesting that people put in some dummy content ("Page not found"/"Something went wrong") seems appropriate to me.
comment:15 by , 12 years ago
This looks really good. Only thing I noticed on a quick skim was that there's an occurrence of HttpReponse
as a typo for HttpResponse
midway through tutorial 3.
Great work!
comment:16 by , 12 years ago
Fixed typo and updated per Russ's feedback except for "another option is to put a templates directory in your polls app, which will be automatically discovered by Django." since #16671 talks about this. I'll a note here when committing that.
comment:17 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Actually, this ticket covers: