Code

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#16758 closed Cleanup/optimization (fixed)

Documentation for TEMPLATE_CONTEXT_PROCESSORS could use additional warning.

Reported by: cyclops Owned by: anonymous
Component: Documentation Version: 1.3
Severity: Normal Keywords:
Cc: yasar11732 Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The documentation for TEMPLATE_CONTEXT_PROCESSORS, at:

https://docs.djangoproject.com/en/1.3/ref/settings/#template-context-processors

could use a warning about over-riding the variable in settings.py, maybe something like:

"Warning - if you set this variable to add your own context processor, it will over-ride the default settings listed above. You must manually add the defaults back into the variable, for them to be available."

Note that this issue has come up and been documented also on StackOverflow: http://stackoverflow.com/questions/2246725/django-template-context-processors

Attachments (4)

0001-Fix-ticket-16758.patch (1.3 KB) - added by yasar11732 3 years ago.
Added neccessary warning.
0001-Added-warning-in-settings.txt.patch (1.2 KB) - added by yasar11732 3 years ago.
Re-written warnings according to remarks.
0001-docs-ref-setting.txt-added-warning.patch (1.1 KB) - added by yasar11732 2 years ago.
16758-4.patch (561 bytes) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Bug to Cleanup/optimization

The situation is identical for all other settings that have a non-empty tuple as default value, notably MIDDLEWARE_CLASSES.

Without a good patch, I'm reluctant to accept this ticket because we can't list all possible errors and this one doesn't look too hard to diagnose.

Last edited 3 years ago by aaugustin (previous) (diff)

comment:2 Changed 3 years ago by julien

  • Triage Stage changed from Unreviewed to Accepted

This could be a useful clarification for beginners, but since it applies to all settings it should be a global note at the top of the page.

comment:3 Changed 3 years ago by yasar11732

  • Owner changed from nobody to yasar11732
  • Status changed from new to assigned

Changed 3 years ago by yasar11732

Added neccessary warning.

comment:4 Changed 3 years ago by yasar11732

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

comment:5 Changed 3 years ago by yasar11732

  • Cc yasar11732 added

comment:6 follow-up: Changed 3 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

Please don't mark your own patches as RFC — the rule is that patches must be reviewed by someone who isn't the author. For more information see the contributing guide in the docs.

Documentation patches are reviewed on a regular basis, so don't worry, your patch will get attention eventually.

comment:7 Changed 3 years ago by bpeschier

  • Patch needs improvement set

Thanks for your contribution; I do have a couple of remarks on the patch:

  1. The suggested text focuses on what people should do instead of WHY they should do it, which feels to me to be belittling the reader a bit.
  2. It's not only beginners who should be cautious, everybody should.
  3. I believe Django is commonly referred to with a capital D.

I would suggest to rewrite it to a more general style ("be careful with settings in settings.py, because ...") and focus on the fact settings.py overrides the default settings.

Changed 3 years ago by yasar11732

Re-written warnings according to remarks.

comment:8 in reply to: ↑ 6 Changed 3 years ago by yasar11732

Replying to aaugustin:

Please don't mark your own patches as RFC — the rule is that patches must be reviewed by someone who isn't the author.

Sorry, I didn't know. I am new to contributing to django.

Replying to bpeschier:

Thanks for your contribution; I do have a couple of remarks on the patch:

Thanks for the remarks. I attached a new patch file.

comment:9 Changed 2 years ago by krzysiumed

The beginning of the warning is:

Be careful with the settings in settings.py, because settings in settings.py override the default values. Overriding some settings may cause ...

Word 'settings' is repeated and repeated. I suggest changing it to something like this

Be careful when you override default values of settings, because it may cause ...


Next, there is

... may cause some core or commonly used functions of Django to stop functioning ...

I'm not sure 'functioning' is good word. I prefer simpler words, for example:

... may cause some core or commonly used functions of Django to stop working correctly ...


yasar11732, you wrote

When overriding default values, you are advised to make sure that none of the functions of Django that you need requires previous setting to work correctly.

Are there features of Django that cannot work correctly with non default values of some settings? I'm asking because I'm new contributor and i don't know everything about Django.


If the setting you are trying to change consists of more than one value (e.g a tuple), ...

I suggest

If the setting you want to change is tuple, list or dictonary, ...

because there are more words 'want' than 'try' in tutorial. I'm not sure if 'is tuple, list or directory' is more clear than 'consists of more than one value'.

Changed 2 years ago by yasar11732

comment:10 Changed 2 years ago by yasar11732

Sorry for late response, I was busy lately. How about latest patch? And about your question, some features need some TEMPLATE_CONTEXT_PROCESSORS to be loaded for example.

comment:11 Changed 2 years ago by krzysiumed

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 2 years ago by aaugustin

  • Patch needs improvement unset

I'm sorry, but I still feel this doesn't match very well the tone of the docs. I'm uploading a new proposal.

Changed 2 years ago by aaugustin

comment:13 Changed 2 years ago by anonymous

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

comment:14 Changed 2 years ago by yasar11732

I have disowned ticket. Someone else might own it if needed.

comment:15 Changed 2 years ago by claudep

Aymeric, I like your version.

comment:16 Changed 2 years ago by timo

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

In [17566]:

Fixed #16758 - Added a warning regarding overriding default settings; thanks cyclops for the suggestion & Aymeric Augustin for the patch.

comment:17 Changed 2 years ago by timo

In [17567]:

[1.3.X] Fixed #16758 - Added a warning regarding overriding default settings; thanks cyclops for the suggestion & Aymeric Augustin for the patch.

Backport of r17566 from trunk.

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.