Hi Omer, I've looked at your code, and while you continue to call it a "refactoring", it's a refactoring with a specific purpose in mind -- to add a new feature.
I disagree that your new code is "clearer" and more "understandable" than the existing code. For starters, it's got more moving parts (pluggable Collector and Loader interfaces, for example), and while it's not too hard to work out how the parts fit together, it isn't intuitively obvious (which is what would be needed to genuinely have a simplification here). On top of all that, what you still haven't done is express is *why* this new feature is required. You clearly want the ability to load settings from a class in some way. It isn't obvious to met at all why this is beneficial or helpful. Lastly, the settings module is one of the warts of Django that most of the core team would like to fix. In this case, "Fixing" it doesn't mean adding more complexity and/or flexibility. It means altering altogether the way settings are handled in Django. There hasn't really been any work in this area beyond some high level talks at DjangoCon etc [1], but adding additional complexity to Django's already complex settings system isn't something we're racing to do without a *really* good reason. [1] http://www.youtube.com/watch?v=0FD510Oz2e4 Yours, Russ Magee %-) On Sat, Mar 23, 2013 at 3:47 PM, Omer Katz <omer.d...@gmail.com> wrote: > So is my example good enough? Have you tried using it? > > בתאריך יום שישי, 15 במרץ 2013 13:17:11 UTC+3, מאת Aymeric Augustin: > >> On 15 mars 2013, at 09:22, Omer Katz <omer...@gmail.com> wrote: >> >> > Doesn't the fact that the patch makes the code clearer is a reason >> enough for a merge (providing that there will be tests attached to it and >> documentation)? >> >> >> Hi Omer, >> >> This patch isn't only a refactoring; it adds a new feature. Otherwise you >> wouldn't be talking about documentation. >> >> Each feature added to Django creates a burden: >> - for users of Django, who must learn to use it; >> - for the core team, who must maintain it for the foreseeable future. >> >> To be accepted, a new feature must: >> (a) provide benefits that clearly outweigh these costs >> (b) not get in the way of future improvements — as much as can be >> foreseen. >> >> Unfortunately (a) the benefits of your PR still aren't clear and (b) >> judging by the discussion, your abstraction doesn't match very well the >> needs of most users, and I suspect it could hinder further efforts to make >> per-environment settings (the actual problem) easier to define. >> >> -- >> Aymeric. >> >> -- > 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. > > > -- 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.
