Opened 5 years ago

Closed 4 years ago

#12248 closed (fixed)

django.template.__init__ code should move to avoid circular imports

Reported by: tomxtobin Owned by: nobody
Component: Template system Version: master
Severity: Keywords:
Cc: tomxtobin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

We maintain an internal branch of Django over at The Onion (based on the Subversion head), and I've been spending this week in the template system (django.template). Changes there have been tricky, since the module is prone to circular import errors as a result of the core template code living in the __init__.py module. I've refactored the module so that this code now lives in django.template.base (similar to how django.db.models is arranged); a patch to this effect is attached. There is no functionality change here, and all existing tests pass. I ask that this be applied to Django directly as it would make it much easier to apply upstream changes to our branch, and would make future refactors (by any party, including Django's own developers) easier as a result.

Change History (6)

comment:1 Changed 5 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Patch needs to be updated following recent updates to the template system.

Secondly, we need to be careful about backwards compatibility here. From inspection, it appears that the current patch is just "mp init.py base.py", plus creating a new init.py that has "from base import *", which seems fine - but the patch also includes a bunch of changes to import usage (changing from django.template import X to from django.template.base import X). I appreciate that these changes are appropriate updates to to use the new locations, but are the otherwise necessary? I just want to make sure I'm not missing a subtle import case that might not be transparent to the change.


comment:2 Changed 5 years ago by korpios

Sure, I can whip up an updated patch; as you pointed out, it's largely just a rename and re-import.

As far as backwards compatibility, those import changes insofar as they take place within the django.template package are indeed necessary as circular import hell (exactly what the restructuring is trying to avoid for future submodules) can result without them. Anything which affects invalid_var_format_string (here, the change to the tests) also needs to refer explicitly to the base module, since that's a module-level attribute, and modifying that value in django.template is not the same thing as modifying it in django.template.base. I honestly don't recall why the import in django.templatetags.i18n needed to change; I'll double-check when I re-do the patch, and omit the change if it isn't necessary.

Other than that, anything outside django.template (including third-party libraries) should be able to go on importing as before, blissfully unaware of the changes.

comment:3 Changed 5 years ago by ubernostrum

  • milestone 1.2 deleted

This isn't a 1.2 issue.

comment:4 Changed 4 years ago by tomxtobin

  • Cc tomxtobin@… added; korpios@… removed
  • Reporter changed from korpios to tomxtobin

comment:5 Changed 4 years ago by tomxtobin

I've brought these changes up to date and placed them on GitHub:

http://github.com/tomxtobin/django/tree/template-import-refactor-t12248

I've additionally sent a pull request to GitHub's django/django:

http://github.com/django/django/pull/3

comment:6 Changed 4 years ago by russellm

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

(In [14722]) Fixed #12248 -- Refactored django.template to get code out of init.py, to help with avoiding circular import dependencies. Thanks to Tom Tobin for the patch.

Note: See TracTickets for help on using tickets.
Back to Top