Code

Opened 2 years ago

Closed 21 months ago

#18715 closed Cleanup/optimization (fixed)

Tutorial Page 3 Refactor

Reported by: pydanny Owned by: pydanny
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 aaugustin 2 years ago.
18715.diff (40.9 KB) - added by timo 21 months ago.
ready for final review?
18715.2.diff (43.4 KB) - added by timo 21 months ago.
Updated per Russ's feedback

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 years ago by pydanny

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to anonymous
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 2 years ago by pydanny

  • Owner changed from anonymous to pydanny
  • Status changed from assigned to new

comment:3 Changed 2 years ago by pydanny

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 2 years ago by pydanny

comment:5 Changed 2 years ago by timo

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 2 years ago by aaugustin

  • Owner changed from pydanny to aaugustin

comment:7 Changed 2 years ago by aaugustin

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.

Version 0, edited 2 years ago by aaugustin (next)

Changed 2 years ago by aaugustin

comment:8 Changed 23 months ago by aaugustin

  • Owner changed from aaugustin to pydanny

Restoring ticket ownership after my review.

comment:9 Changed 23 months ago by timo

  • 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 22 months ago by pydanny

What should I do to move this ticket forward?

comment:11 Changed 22 months ago by timo

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

comment:12 Changed 21 months ago by timo

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 21 months ago by timo

ready for final review?

comment:13 Changed 21 months ago by timo

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 21 months ago by russellm

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 21 months ago by carljm

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 21 months ago by timo

Updated per Russ's feedback

comment:16 Changed 21 months ago by timo

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 21 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 07abb7a6b7af2c45be553acf08d85cd2d72057ad:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.