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

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

Conversation

jayaddison
Copy link
Contributor

This reverts commit 5d13215.

Feature or Bugfix

  • Revert

Purpose

Detail

  • Updating templated filename(s) in alabaster and other themes would require updating the minimum supported Sphinx version for each of those themes -- because they'd become incompatible with older versions of Sphinx.
  • If anyone is using Sphinx to copy filenames that already have a .jinja suffix, and expect those files to be copied statically (without any templating applied), then Support and prefer .jinja to _t for static templates #11165 would break those workflows.

These are manageable concerns, but they'd be better addressed if the changes are communicated clearly, and I think that at least one additional release -- and then a good duration of time to allow affected extensions/sites to adjust accordingly -- prior to supporting the .jinja suffix could make sense to achieve that.

Relates

@jayaddison jayaddison force-pushed the pr-11165-revert/temporarily-undo-jinja-suffix-support branch from 60972ca to 7279428 Compare April 15, 2023 17:03
Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

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

Thanks for your concern. One worry is that third-party extensions may be enticed by deprecation warning to adopt .jinja suffix thus breaking usage with older Sphinx for end-users. Let's way for other maintainers opinion.

@AA-Turner
Copy link
Member

One worry is that third-party extensions may be enticed by deprecation warning to adopt .jinja suffix thus breaking usage with older Sphinx for end-users.

I agree with this, at the very least we should likely not issue the deprecation warning initially (perhaps control via an opt-in environment variable?), and improve the wording to be more directive.

If anyone is using Sphinx to copy filenames that already have a .jinja suffix, and expect those files to be copied statically (without any templating applied), then #11165 would break those workflows.

I don't think this is a realistic concern (happy to be proven wrong!) -- this suggests that there would be a second subsequent application of Jinja by some non-Sphinx tool? I also don't know what the useful work-around would be for those wanting to preserve current plain copying behaviour.

A

@AA-Turner
Copy link
Member

I think we should revert iff we have evidence of the .jinja copying behaviour in the wild as of today -- otherwise we should just be more cautious with deprecation warnings etc.

A

@jayaddison
Copy link
Contributor Author

One worry is that third-party extensions may be enticed by deprecation warning to adopt .jinja suffix thus breaking usage with older Sphinx for end-users.

I agree with this, at the very least we should likely not issue the deprecation warning initially (perhaps control via an opt-in environment variable?), and improve the wording to be more directive.

Ok. I've re-read the deprecation policy, and would prefer to re-use existing Sphinx and Python mechanisms (that is: I'm not sure I'm keen on a custom environment variable).

If we update the warning message to emphasize the pending aspect of the deprecation: providing guidance on how to filter the warning, instead of recommending switching the suffix immediately: would that be acceptable?

(the idea being: the PendingDeprecation stage is deployment of compatibility and communicating the future change, the Deprecation stage itself is pushing for the migration to occur, and then finally we remove the feature)

If anyone is using Sphinx to copy filenames that already have a .jinja suffix, and expect those files to be copied statically (without any templating applied), then #11165 would break those workflows.

I don't think this is a realistic concern (happy to be proven wrong!) -- this suggests that there would be a second subsequent application of Jinja by some non-Sphinx tool? I also don't know what the useful work-around would be for those wanting to preserve current plain copying behaviour.

I'm not sure how realistic it is either, but it was an additional doubt. I haven't seen it in practice anywhere; my worry was webservers configured to render templates dynamically. As a workaround, I think that adding a duplicate .jinja suffix could work.

@AA-Turner AA-Turner mentioned this pull request Apr 21, 2023
@AA-Turner
Copy link
Member

As a workaround, I think that adding a duplicate .jinja suffix could work.

Yes, but every single Jinja construct would need to be escaped! (Or: we add special handling for this case, I'm not sure which is less-surprising.)

A

@jayaddison
Copy link
Contributor Author

As a workaround, I think that adding a duplicate .jinja suffix could work.

Yes, but every single Jinja construct would need to be escaped! (Or: we add special handling for this case, I'm not sure which is less-surprising.)

True - that'd be both mindbending and quite an obscure way to structure part of an application.

Ok: perhaps for .jinja files, simply a warning message to say that .jinja files will be interpreted as templates in future -- a message that hopefully few if any people will encounter, and that can be removed when support for the old suffix is removed?

@AA-Turner
Copy link
Member

@humitos -- does Read The Docs' data include what files are copied or in source trees (and therefore the proportion of files with a particular suffix)? No worries if not, thought I'd ask you to try and get some empirical data here!

A

@AA-Turner AA-Turner merged commit 59de8d5 into sphinx-doc:master Apr 23, 2023
22 checks passed
@AA-Turner
Copy link
Member

I've decided to defer this -- I still think that we can press ahead with the work, but some changes need to be made (most notably related to when and how we warn users), and I want to release 6.2.

A

@jayaddison jayaddison deleted the pr-11165-revert/temporarily-undo-jinja-suffix-support branch April 23, 2023 20:09
@jayaddison
Copy link
Contributor Author

Thanks @AA-Turner - I'd still like to make this change too, but feel like I could do with taking a week or two away from the code before returning to it for a fresh attempt.

It seems like it should be straightforward when planned and deployed carefully, but I got into a bit of a tangle somehow while (not) thinking it through.

@AA-Turner AA-Turner added this to the 6.2.0 milestone Apr 29, 2023
@jayaddison
Copy link
Contributor Author

jayaddison commented May 12, 2023

I'm almost feeling ready to take another look at this - maybe a little more mental preparation first, though. I'd like to make sure some of the linkchecker changes I've been working on are all tidied up and in good shape before re-attempting this, to make sure I can focus (relatively!) clearly on it.

Given the reaction to the deprecation warning in the prerelease phase for 6.2.0, I think that @mgeier's original suggestion to roll-out the migration functionality before a deprecation warning is issued would make sense.

So, I'm currently thinking:

  • Phase 1: Add support for the .jinja templates, and allow time for that to be deployed.
  • Phase 2: Enable a deprecation warning when the legacy _t suffix is used.
  • Phase 3: Drop support for the legacy _t suffix.

@mgeier
Copy link
Contributor

mgeier commented May 21, 2023

I would like to (again) suggest to support both suffixes indefinitely.

Phase 1 sounds good, Phase 2 and 3 can be postponed indefinitely.

@jayaddison
Copy link
Contributor Author

What's the reason you'd prefer to support the _t suffix indefinitely (including across major version increments), @mgeier?

(I'm OK with phase 2 and 3 taking a while, but I think it's generally likely to be beneficial and clearer to users and various other ecosystem elements if the purpose of the template files is represented by an appropriate file suffix, and that we should encourage people in that direction)

@mgeier
Copy link
Contributor

mgeier commented May 22, 2023

What's the reason you'd prefer to support the _t suffix indefinitely?

To avoid friction for casual users.

I think [citation needed] that there are many users who have docs generated by Sphinx, but their docs rarely change and those people might have other things to worry about than regularly checking if a Sphinx update has broken their docs.

And if they are using an unmaintained Sphinx extension that uses _t (but is otherwise still fully functional), they might not be able to fix the problem at all.

Note that [citation needed] many Sphinx users are not programmers, so a "simple" change from _t to .jinja might be more than they are willing/able to do.

Of course some breakage is unavoidable in a project as dynamic as Sphinx, but I think all contributors and maintainers should strive to reduce the number and frequency of breakages. And this is a prime example of a 100% avoidable breakage.

I think the Sphinx docs can simply recommend using .jinja, they don't even have to mention _t anymore. In the code, a comment can explain that _t is "for backwards compatibility", and that's all that's needed.

And sure, at some point the _t support can be removed if it turns out to become a maintenance hurdle. But this may not happen for another 5 to 10 years. Or never.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants