Support and prefer `.jinja` to `_t` for static templates #11165

jayaddison · 2023-01-31T16:20:40Z

Feature or Bugfix

Refactoring

Purpose

This is a pedantic change that relates to nedbat/coveragepy#1488, where coveragepy emits some warnings because it's unclear based on the file suffix whether some template files that contain Python code (and are processed by Jinja) indeed are Python code.

The suggested changes here probably only make sense if the corresponding templates are only expected to be used internally by Sphinx itself. If the template filenames are part of any kind of implicit interface for external libraries/applications, then this change might be more disruptive than the small benefit that it could provide.

Detail

Where templates named with a _t suffix exist that contain Jinja templates, replace the suffix with .jinja2
.j2 was considered as an alternative suffix - .jinja2 seems less ambiguous
- 2023-02-19: Also, the Jinja2 project has been renamed to Jinja

Relates

[low-prio] Possible bug: files without extensions misidentified as Python? nedbat/coveragepy#1488

mgeier · 2023-01-31T17:44:33Z

This would break all extensions that use the _t convention and copy_asset_file() (as described in the docs: https://www.sphinx-doc.org/en/master/development/theming.html#add-your-own-static-files-to-the-build-assets).

So please don't do that.

jayaddison · 2023-01-31T18:43:48Z

Thanks @mgeier - I wasn't aware of that.

I think that results in two possible paths to follow: either I'll abandon these changes, or I'll include compatibility with the existing suffix for extensions that rely on that.

(I'm leaning towards the former at the moment, but will spend some more time thinking about it)

AA-Turner · 2023-01-31T19:05:57Z

I think that results in two possible paths to follow: either I'll abandon these changes, or I'll include compatibility with the existing suffix for extensions that rely on that.

I support the change but we'd have to include a deprecation period of perhaps a year or longer.

A

…generic/ambiguous '_t' suffix

sphinx/util/fileutil.py

…PEP0563 for pre-py310)

jayaddison · 2023-02-06T19:00:53Z

Although this is ready-for-review, I'm also trying to gauge my level of commitment in terms of supporting and migrating existing extensions.

Generally I think the best approach would be to try to identify as many extensions as possible that are reliant on the existing template suffix format and to offer pull requests with relevant migrations for them (hopefully in most cases those would be straightforward). I might need to spend a bit of time to recharge before starting that analysis, though.

…a', so let's simplify the filename extension accordingly

…-argument method call parameter list)

…for '.jinja2', is now: six characters for '.jinja') Note: cannot yet simplify this code by using str.removesuffix, since that was introduced in py39 and we are maintaining py38 support

jayaddison · 2023-02-20T12:52:38Z

I've run some (admittedly legacy) GitHub code searches to find potentially-affected extensions. Very few use the _t suffix on templates, and they do not appear to be widely-used extensions. A few other extensions could be configured or read from input files (or files from directories) that contain the template suffix.

In cases that are popular OR definitely affected (regardless of maintenance status) I'll offer pull requests to adjust the behaviour.

So far I don't think that there are many valid use cases for dynamic content rendering during asset file copying, but there may be valid use cases I'm yet to learn about.

jayaddison · 2023-02-26T12:07:36Z

So far I don't think that there are many valid use cases for dynamic content rendering during asset file copying, but there may be valid use cases I'm yet to learn about.

Following on from that: I'm still unaware of many good use cases for this copy-and-apply-templating in the Sphinx copy_asset_file method. There could be some out there (and I wonder about non-free/public code in particular), but I haven't yet figured out what they could be. Even if valid use cases do exist, I wonder whether custom code to apply a Jinja template and then use Sphinx code to perform the copy could be clearer and more explicit about the intent of the code, without any loss of functionality.

That's making me wonder whether instead of suggesting a .jinja template suffix as an alternative, it could make more sense to remove the templating functionality from copy_asset_file and only allow it to perform static file copying (probably a major-api-version change required if so, though?).

mgeier · 2023-03-19T20:35:38Z

I've run some (admittedly legacy) GitHub code searches to find potentially-affected extensions. Very few use the _t suffix on templates, and they do not appear to be widely-used extensions.

The suggested change here will not only affect extensions that use copy_asset_file()!

It will even not only affect extensions, but also themes and plain old Sphinx users who want to overwrite an existing file in _static (or _templates) with a file that uses Jinja markup.

Here are examples for the (at least) three different cases:

Extension using copy_asset() to copy files from https://github.com/spatialaudio/nbsphinx/tree/master/src/nbsphinx/_static
Theme just providing a few static files (no Python code involved at all!): https://github.com/mgeier/insipid-sphinx-theme/tree/master/src/insipid_sphinx_theme/insipid/static
Normal Sphinx documentation, just overwriting a template that's normally provided by Sphinx (no Python code used except for defining templates_path in conf.py): https://github.com/spatialaudio/nbsphinx/pull/719/files#diff-fcf9d762319bf115a8c6cdad49abf75fd39d76d4d1a2d44ed7f114653968a6d4

Those are just examples where I myself have used the _t suffix. I'm sure there are many more examples on Github (but maybe hard to search for) and even more in non-public repositories.

I was wondering though: since it seems to be reasonably simple to support both _t and .jinja for a deprecation period, why not support both indefinitely and never deprecate them?

jayaddison · 2023-03-19T21:13:53Z

Thank you for looking into this further @mgeier. I became sidetracked by one particular detail, as I often do, and forgot to look at the rest of the picture.

I was wondering though: since it seems to be reasonably simple to support both _t and .jinja for a deprecation period, why not support both indefinitely and never deprecate them?

That seems like a good approach to me 👍

sphinx/util/fileutil.py

… to hold the suffix string elements Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

jayaddison · 2023-04-07T10:12:38Z

A pedantic observation:

The copy_asset methods check for filenames ending in _t / .jinja in a case-insensitive way
The code to apply templating during output generation (for example, LaTeX templating) check for existence of templates using case-sensitive inputs

I don't think that's likely to cause huge problems; on case-sensitive filesystems I doubt that people would prefer to use _T or .JINJA as file suffixes. Reality being what it is, there are probably some rare cases where that would occur, though.

…me' method

jayaddison · 2023-04-07T10:23:05Z

I don't think that's likely to cause huge problems; on case-sensitive filesystems I doubt that people would prefer to use _T or .JINJA as file suffixes. Reality being what it is, there are probably some rare cases where that would occur, though.

I don't think it's worth the additional filesystem existence checks to look for these. If someone does encounter a problem due to uppercase file extensions on a case-sensitive system, then hopefully they can resolve that locally and harmlessly by renaming the files. If not, then with an issue report we can consider adjusting the code appropriately.

jayaddison · 2023-04-07T10:24:12Z

Ok, thanks for bearing with me while I hold tedious public conversations with myself @AA-Turner. I think I'm now comfortable that this changeset is ready to merge 👍

mgeier · 2023-04-07T11:56:38Z

CHANGES

@@ -23,6 +23,9 @@ Deprecated
 ----------

 * #11247: Deprecate the legacy ``intersphinx_mapping`` format
+* #11165: The ``_t`` suffix for templating (``sphinx.util.copy_asset()``,
+  ``sphinx.util.copy_asset_file()`` and built-in templates) is now deprecated.


I would prefer if they would not yet be called "deprecated".

I think consensus was to mark them as "pending deprecation"?

Ok, yep: I'll update the CHANGES entry (and this pull request title) to say "pending deprecation" instead.

Sorta-related: reading back through some of the previous commentary, it looks like I misread one of your previous messages:

I was wondering though: since it seems to be reasonably simple to support both _t and .jinja for a deprecation period, why not support both indefinitely and never deprecate them?

That seems like a good approach to me 👍

Somehow I didn't read/understand the never deprecate them part of that.

To me, the goal here is to make the purpose and content of the template files less ambiguous. Currently _t indicates that they're templates, but doesn't say much about what kind of templating that is. .jinja indicates both template, and a templating language that's in use.

(so, correcting myself: supporting both sounds good, but I would like to suggest deprecating _t at some point)

If we supported both suffixes _t and .jinja equally: would there be any situations where we'd recommend using _t instead of .jinja?

mgeier · 2023-04-07T11:59:16Z

sphinx/ext/imgmath.py

-            return LaTeXRenderer().render(template, variables)
-
-    return LaTeXRenderer(templates_path).render(template_name, variables)
+        # TODO: remove "_t" template suffix support after 2025-04-06


I think support should not be removed at that time, but the deprecation period should start at that time!

At the given date, a DeprecationWarning should be created, and after another year or two support for _t can be removed.

mgeier · 2023-04-07T11:59:43Z

sphinx/util/fileutil.py

+    """Given an input filename:
+    If the input looks like a template, then return the filename output should
+    be written to.  Otherwise, return no result (None)."""
+    # TODO: remove "_t" template suffix support after 2025-04-06


See above, the deprecation period should start at that date.

mgeier · 2023-04-07T12:00:10Z

sphinx/util/fileutil.py

+    # TODO: remove "_t" template suffix support after 2025-04-06
+    if filename.lower().endswith('_t'):
+        warnings.warn(
+            f"{filename!r}: filename suffix '_t' for templates is deprecated. "


Suggested change

f"{filename!r}: filename suffix '_t' for templates is deprecated. "

f"{filename!r}: filename suffix '_t' for templates will become deprecated. "

mgeier · 2023-04-07T12:00:43Z

sphinx/writers/latex.py

-                                 template_name)
-            if path.exists(template):
-                return renderer.render(template, variables)
+            # TODO: remove "_t" template suffix support after 2025-04-06


As above, this should be the start of the deprecation period.

mgeier · 2023-04-07T12:01:13Z

tests/test_build_latex.py

@@ -1379,6 +1379,10 @@ def test_latex_table_custom_template_caseA(app, status, warning):
    result = (app.outdir / 'python.tex').read_text(encoding='utf8')
    assert 'SALUT LES COPAINS' in result

+    # # TODO: remove "_t" template suffix support after 2025-04-06


same as above

mgeier · 2023-04-07T12:01:24Z

tests/test_util_fileutil.py

+
+
+def test_legacy_template_basename():
+    # TODO: remove "_t" template suffix support after 2025-04-06


same as above

mgeier · 2023-04-07T12:03:26Z

Is it really necessary to ever remove support for _t?

It looks like minimal additional code, and the whole deprecation mechanism sure seems much more complicated than simply keeping both _t and .jinja, doesn't it?

AA-Turner · 2023-04-07T16:41:00Z

I've centralised the 'deprecation' calls to sphinx.deprecation, and added documentation on how themes can silence the warning, which is important while Sphinx 6.1 and earlier are still supported by said themes (as you'd need to maintain two identical templates, or do some dynamic renaming, or etc -- which static templates are designed to avoid).

I've also updated the documentation in (what will become) https://www.sphinx-doc.org/en/master/development/theming.html.

A

AA-Turner · 2023-04-07T17:12:16Z

Thanks @jayaddison!

A

jayaddison · 2023-04-07T17:12:53Z

Thanks again @AA-Turner - I'll stay around to watch for issues related to this (and should probably re-read the deprecation policy a few times during the process).

jayaddison · 2023-04-15T14:51:55Z

I've begun thinking about an edge-case today that I haven't yet found in practice, but it does still worry me:

What if some Sphinx sites/extensions contain static files that intentionally already use a .jinja suffix that aren't intended to be evaluated during file copying?

With this change, those files would be evaluated and the results written to a file without the extension, possibly a breaking change in those cases.

jayaddison · 2023-04-15T15:23:32Z

Even though I'd be slightly annoyed with myself for reverting this change (for clearly not planning it thoroughly), perhaps this should not be included in 6.2.0 - with my previous comment in mind, I think a better way to phase it in would be:

In release A, add a warning during copying of any files that have a .jinja suffix
In release B some time later, begin supporting .jinja as a template suffix
In release C, some time later again, drop support for the legacy _t template

Step one would allow time for people who do use .jinja static files for any reason to think about a migration path, and ideally to let us know what the reasons for that are.

Reasoning:

Perhaps I'm overthinking it, but since this is something I'm contributing in personal time (I do make contributions that I consider work-related here too, but this isn't one of them), and would continue to complete this in personal time, I'd prefer to choose a path that's likely to be as minimally-stressful for users (and therefore myself) as possible, even if that takes longer.

If it is included in 6.2.0 anyway though, I'll make time available to support these changes as best possible.

…phinx-doc#11165)" This reverts commit 5d13215.

jfbu · 2023-04-16T11:05:57Z

Even though I'd be slightly annoyed with myself for reverting this change (for clearly not planning it thoroughly), perhaps this should not be included in 6.2.0

@jayaddison Thanks for all your work on this. I understand your concern and maybe indeed we can delay it so I will add a LGTM to your reverting PR, and wait for @AA-Turner opinion. My presence here will be very sparse for about 4 weeks.

jayaddison · 2023-04-16T11:55:19Z

Thank you, @jfbu.

…11165)" (#11329) This reverts commit 5d13215.

jayaddison marked this pull request as draft February 1, 2023 06:41

Templating: use '.jinja2' suffix on Jinja2 template files instead of …

3e735ee

…generic/ambiguous '_t' suffix

jayaddison force-pushed the templating/jinja2-suffix branch from 3bfb7b4 to 3e735ee Compare February 3, 2023 23:01

jayaddison commented Feb 3, 2023

View reviewed changes

sphinx/util/fileutil.py Outdated Show resolved Hide resolved

jayaddison marked this pull request as ready for review February 3, 2023 23:06

ruff suggestion: adopt union typing syntax (with support provided by …

5ce9483

…PEP0563 for pre-py310)

jayaddison mentioned this pull request Feb 4, 2023

PythonFileReporter: do not expect files without a filename extension to contain Python code nedbat/coveragepy#1489

Closed

Merge branch 'master' into templating/jinja2-suffix

168cb23

jayaddison marked this pull request as draft February 19, 2023 15:49

jayaddison changed the title ~~Templating: use '.jinja2' suffix on Jinja2 template files instead of generic/ambiguous '_t' suffix~~ Templating: use '.jinja' suffix on Jinja template files instead of generic/ambiguous '_t' suffix Feb 19, 2023

jayaddison added 4 commits February 19, 2023 16:16

Update file extension: the 'jinja2' project has been renamed to 'jinj…

4a8a339

…a', so let's simplify the filename extension accordingly

Merge branch 'master' into templating/jinja2-suffix

eb9938a

Fixup: ruff lint error (missing trailing comma after multi-line multi…

43f8b2b

…-argument method call parameter list)

Fixup: adjust trim-length for template suffix (was: seven characters …

b2889de

…for '.jinja2', is now: six characters for '.jinja') Note: cannot yet simplify this code by using str.removesuffix, since that was introduced in py39 and we are maintaining py38 support

jayaddison added 2 commits March 21, 2023 20:07

Localization: undo changes to resource files

a720933

Merge branch 'master' into templating/jinja2-suffix

6f99cef

jayaddison marked this pull request as ready for review March 21, 2023 20:55

jayaddison changed the title ~~Templating: use '.jinja' suffix on Jinja template files instead of generic/ambiguous '_t' suffix~~ Templating: support and migrate to '.jinja' suffix on Jinja template files in preference to deprecated '_t' suffix Apr 5, 2023

AA-Turner reviewed Apr 5, 2023

View reviewed changes

sphinx/util/fileutil.py Outdated Show resolved Hide resolved

Template suffix iteration: use the 'tuple' datatype instead of 'list'…

d0aa68a

… to hold the suffix string elements Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Documentation: add explanatory docstring to utility '_template_basena…

299dbed

…me' method

mgeier reviewed Apr 7, 2023

View reviewed changes

AA-Turner added 4 commits April 7, 2023 17:34

Adjust date, add helper function

c3b5cb9

Add tests

6600615

Update theming.rst

9327357

Update CHANGES

cccdfaf

AA-Turner added 2 commits April 7, 2023 17:42

left -> removed

cb9ea44

credits

9a4410a

AA-Turner changed the title ~~Templating: support and migrate to '.jinja' suffix on Jinja template files in preference to deprecated '_t' suffix~~ Support and prefer .jinja to _t for static templates Apr 7, 2023

AA-Turner merged commit 5d13215 into sphinx-doc:master Apr 7, 2023

jayaddison deleted the templating/jinja2-suffix branch April 7, 2023 17:12

AA-Turner added this to the 6.2.0 milestone Apr 8, 2023

jayaddison added a commit to jayaddison/sphinx that referenced this pull request Apr 15, 2023

Revert "Support and prefer .jinja to _t for static templates (s…

60972ca

…phinx-doc#11165)" This reverts commit 5d13215.

This was referenced Apr 15, 2023

Rename alabaster.css_t to alabaster.css.jinja sphinx-doc/alabaster#204

Closed

Revert "Support and prefer .jinja to _t for static templates (#11165)" #11329

Merged

jayaddison added a commit to jayaddison/sphinx that referenced this pull request Apr 15, 2023

Revert "Support and prefer .jinja to _t for static templates (s…

7279428

…phinx-doc#11165)" This reverts commit 5d13215.

AA-Turner pushed a commit that referenced this pull request Apr 23, 2023

Revert "Support and prefer .jinja to _t for static templates (#…

59de8d5

…11165)" (#11329) This reverts commit 5d13215.

github-actions bot locked as resolved and limited conversation to collaborators May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support and prefer `.jinja` to `_t` for static templates #11165

Support and prefer `.jinja` to `_t` for static templates #11165

jayaddison commented Jan 31, 2023 •

edited

Loading

mgeier commented Jan 31, 2023

jayaddison commented Jan 31, 2023

AA-Turner commented Jan 31, 2023

jayaddison commented Feb 6, 2023

jayaddison commented Feb 20, 2023

jayaddison commented Feb 26, 2023

mgeier commented Mar 19, 2023

jayaddison commented Mar 19, 2023

jayaddison commented Apr 7, 2023

jayaddison commented Apr 7, 2023

jayaddison commented Apr 7, 2023

mgeier Apr 7, 2023

jayaddison Apr 7, 2023

jayaddison Apr 7, 2023

mgeier Apr 7, 2023

mgeier Apr 7, 2023

mgeier Apr 7, 2023

mgeier Apr 7, 2023

mgeier Apr 7, 2023

mgeier Apr 7, 2023

mgeier commented Apr 7, 2023

AA-Turner commented Apr 7, 2023

AA-Turner commented Apr 7, 2023

jayaddison commented Apr 7, 2023

jayaddison commented Apr 15, 2023

jayaddison commented Apr 15, 2023

jfbu commented Apr 16, 2023

jayaddison commented Apr 16, 2023

	f"{filename!r}: filename suffix '_t' for templates is deprecated. "
	f"{filename!r}: filename suffix '_t' for templates will become deprecated. "



		def test_legacy_template_basename():
		# TODO: remove "_t" template suffix support after 2025-04-06

Support and prefer .jinja to _t for static templates #11165

Support and prefer .jinja to _t for static templates #11165

Conversation

jayaddison commented Jan 31, 2023 • edited Loading

Feature or Bugfix

Purpose

Detail

Relates

mgeier commented Jan 31, 2023

jayaddison commented Jan 31, 2023

AA-Turner commented Jan 31, 2023

jayaddison commented Feb 6, 2023

jayaddison commented Feb 20, 2023

jayaddison commented Feb 26, 2023

mgeier commented Mar 19, 2023

jayaddison commented Mar 19, 2023

jayaddison commented Apr 7, 2023

jayaddison commented Apr 7, 2023

jayaddison commented Apr 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgeier commented Apr 7, 2023

AA-Turner commented Apr 7, 2023

AA-Turner commented Apr 7, 2023

jayaddison commented Apr 7, 2023

jayaddison commented Apr 15, 2023

jayaddison commented Apr 15, 2023

jfbu commented Apr 16, 2023

jayaddison commented Apr 16, 2023

Support and prefer `.jinja` to `_t` for static templates #11165

Support and prefer `.jinja` to `_t` for static templates #11165

jayaddison commented Jan 31, 2023 •

edited

Loading