On Fri, Mar 15, 2013 at 3:36 PM, Omer Katz <omer.d...@gmail.com> wrote:
> Why would you call them magic? > Why does allowing extensibility for those who need it is a bad idea? > You will be doing it explicitly anyway by providing a SettingsCollector > class to the Settings class' constructor. > If are doing it, you should know what you are doing. > . Is my code harder to debug or understand than the current code? I > strongly disagree here. The current Settings class clearly violates > SRP<http://en.wikipedia.org/wiki/Single_responsibility_principle>. > It holds the settings, validates them and collect them. What's not easy to > understand about SettingsCollector? You can understand by it's name what > exactly it does. > > "The only reason for having these SettingsCollectors in core is to allow > redistributable apps like django-configurations to do magic. That is, so > they can say, "Install django-configurations, and suddenly every setting > can be overridden from the environment!" This seems like a bad idea, versus > "Install django-configurations, and now you have a new tool in your toolbox > to put in settings.py that imports from the environment!"" > Why exactly this is a bad idea? Since when extensibility is a bad idea in > software development? > You just say you are against it but you don't provide a good reason other > than it's easier to explain to others what's wrong if something is wrong. > The current behavior will stay as the default behavior *at least *until > Django 2.0 if not forever so "it's easier to explain to others what's > wrong" is not a valid argument in my opinion. > If you are writing your own SettingsCollector you probably know what you > are doing. > If we'll introduce other types of SettingsCollectors in Django then we > won't introduce them in the Getting Started documentation until a very late > stage so that newcomers can understand the default behavior but that's > another issue for later on. > > For now I want to focus on the pull request itself. > Does the refactoring makes the code clearer in it's intension? > Does it allow extensibility when it is required? > Does it maintain the default behavior for most Django developers? > > I believe that the answer to all of those questions is yes it does. > I'd definitely argue the first point - that it makes the code clearer. If you break up your settings file using normal Python imports, the order of evaluation of a settings file is clear, and can be followed by anyone that understands the normal Python import process. In order to use your patch, you need to understand how settings modules will be combined, and in what order, and with what precedence. It relies upon implicit knowledge of the mode of operation of the collector, rather than the explicit behaviour of a built-in language feature. As for the second point -- As I've said previously, I'd argue that yes, it allows extensibility, but not on any axis that, in my experience, requires it. I've seen a range of problems related to the structure of settings files, but none of them require the sort of structure that you're making easy to achieve here. If you want to propose a way to make it easy to separate production from development settings, or a way to keep private settings (like passwords and SECRET_KEY) out of repositories, I'm all ears -- but I suspect these problems are more an issue of documenting conventions than of adding features. The last point is moot - backwards compatibility is definitely required for any feature going into core, but that doesn't mean that just because a feature is backwards compatible it will be added to core. Yours, Russ Magee %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers?hl=en. For more options, visit https://groups.google.com/groups/opt_out.
