Opened 4 years ago

Closed 4 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)

18715.patch (31.0 KB) - added by Aymeric Augustin 4 years ago.
18715.diff (40.9 KB) - added by Tim Graham 4 years ago.
ready for final review?
18715.2.diff (43.4 KB) - added by Tim Graham 4 years ago.
Updated per Russ's feedback

Download all attachments as: .zip

Change History (20)

comment:1 Changed 4 years ago by Daniel Greenfeld

Owner: changed from nobody to anonymous
Status: newassigned

comment:2 Changed 4 years ago by Daniel Greenfeld

Owner: changed from anonymous to Daniel Greenfeld
Status: assignednew

comment:3 Changed 4 years ago by Daniel Greenfeld

Actually, this ticket covers:

  • Changing the order of instructions. Views and Urls are 'tightly coupled' in the code and should be so in the tutorial.
  • Views and Templates should be taught not via throwing errors and then fixing them.
  • Good practices on URLconf are taught from the beginner per Jessica McKellar. For example, we begin with polls/urls.py in place rather than adding them at the end.

comment:4 Changed 4 years ago by Daniel Greenfeld

comment:5 Changed 4 years ago by Tim Graham

Has patch: set
Triage Stage: UnreviewedAccepted

comment:6 Changed 4 years ago by Aymeric Augustin

Owner: changed from Daniel Greenfeld to Aymeric Augustin

comment:7 Changed 4 years ago by Aymeric Augustin

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 lose 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.

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

Changed 4 years ago by Aymeric Augustin

Attachment: 18715.patch added

comment:8 Changed 4 years ago by Aymeric Augustin

Owner: changed from Aymeric Augustin to Daniel Greenfeld

Restoring ticket ownership after my review.

comment:9 Changed 4 years ago by Tim Graham

Cc: timograham@… 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:10 Changed 4 years ago by Daniel Greenfeld

What should I do to move this ticket forward?

comment:11 Changed 4 years ago by Tim Graham

Would you like to make some tweaks based on my comments (or let me know if you disagree with them)?

comment:12 Changed 4 years ago by Tim Graham

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!

Changed 4 years ago by Tim Graham

Attachment: 18715.diff added

ready for final review?

comment:13 Changed 4 years ago by Tim Graham

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 Changed 4 years ago by Russell Keith-Magee

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 Changed 4 years ago by Carl Meyer

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!

Changed 4 years ago by Tim Graham

Attachment: 18715.2.diff added

Updated per Russ's feedback

comment:16 Changed 4 years ago by Tim Graham

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 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 07abb7a6b7af2c45be553acf08d85cd2d72057ad:

Fixed #18715 - Refactored tutorial 3. Thank-you Daniel Greenfeld!

Note: See TracTickets for help on using tickets.
Back to Top