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

Convert global settings to list subclass #3752

Closed
wants to merge 3 commits into from
Closed

Convert global settings to list subclass #3752

wants to merge 3 commits into from

Conversation

ryanhiebert
Copy link
Contributor

The style is unclear and inconsistent about whether the settings are preferred to be lists or tuples. A discussion in the mailing list seems to indicate a consensus that lists are preferred, but backward compatibility may be a concern that needs to be considered.

I've implemented a ConfList list subclass that is able to act like a tuple in some tricky cases that might otherwise throw errors. Based on recent posts on the mailing list, the ConfList subclass is likely to be discarded in favor of simply documenting the backward incompatibility.

I updated the default template to use lists, and I've started working my way through the tutorial, and then on to the rest of the docs. There's still a bunch more to go through.

It's necessary to use a list subclass for now so that we can deprecate
using the settings as tuples without breaking code gratuitiously.
@carljm
Copy link
Member

carljm commented Dec 18, 2014

This looks really good! Thanks for the work! Since (as you noted) @freakboy3742 is comfortable doing this as a simple change without a deprecation path, and I am as well, I think I'll close this PR -- but if something crops up to convince us that a deprecation path is needed, we'll definitely resurrect this.

(Or if you want to extend this PR to complete hunting through the docs for tuple examples, and remove ConfList, I'll reopen it.)

@carljm carljm closed this Dec 18, 2014
@@ -430,7 +430,7 @@ look like this:
'django.contrib.messages',
'django.contrib.staticfiles',
'polls',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a stated goal of moving from tuples to lists is avoiding the trailing comma problem in tuples, maybe remove the trailing comma from the list in the example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing wrong with trailing commas; I think in multi line lists trailing commas are much preferable. The problem with tuples is the requirement of a trailing comma in a length-one tuple (which is likely not multi line).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nothing wrong with them. I was just surprised to see this after reading the django-developers thread, since half of the impetus for this change is to prevent the missing trailing commas problem with tuples. Just a minor observation. Fine either way.

@ryanhiebert ryanhiebert deleted the list-settings branch April 30, 2015 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants