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

Rename alabaster.css_t to alabaster.css.jinja #204

Closed
wants to merge 2 commits into from

Conversation

pllim
Copy link

@pllim pllim commented Apr 14, 2023

To fix #203

Copy link

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

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

Hi @pllim - could you update the filename to alabaster.css.jinja?

(Sphinx copies theme-related files like this one as part of a Python copy_asset method, and it will remove the suffix - either _t or .jinja - from any files considered templates that end with those. in this case, we'd like the destination file to be alabaster.css, according to the alabaster theme.conf, so either alabaster.css_t or alabaster.css.jinja should work)

@pllim
Copy link
Author

pllim commented Apr 15, 2023

Sure thing, I have renamed it. I apologize for misinterpreting the warning message.

If you want me to squash, let me know. Otherwise, I would recommend to use the "squash and merge" button here. Thanks!

@pllim pllim changed the title Rename alabaster.css_t to alabaster.jinja Rename alabaster.css_t to alabaster.css.jinja Apr 15, 2023
@jayaddison
Copy link

Thanks @pllim - looking further into the build warnings you saw has made me wonder whether this particular change (recommending the .jinja suffix instead of _t) is ready-for-release in 6.2.0 - I wrote the change, and the Sphinx team has been supportive.. but I'm feeling a bit uncertain.

I mention that mainly because this change should only be applied if Sphinx 6.2.0 does decide to include support for .jinja (otherwise, we risk breaking the alabaster theme).

I've written some more thoughts here: sphinx-doc/sphinx#11165 (comment) - please let me know if I can explain more.

@pllim
Copy link
Author

pllim commented Apr 15, 2023

For now, we have ignored this warning in our test suite downstream, so there is no rush on my end. However, I will be interested to see how Sphinx would resolve this dilemma. Thanks for the link!

@jayaddison
Copy link

Thank you @pllim (and thank you also @bsipocz for reporting the problem).

I think that my preference is to rollback the change and then re-consider it for inclusion in a later release. That might be a very-cautious approach, but I'm volunteering on this so I'd also like to avoid causing too much frustration (both to myself and others).

I'll spend some more time thinking about the options and waiting for any further commentary in the Sphinx project.

@AA-Turner
Copy link
Member

This would break support for all released versions of Sphinx! I'll close for now, discussion continues on sphinx-doc/sphinx#11329.

A

@AA-Turner AA-Turner closed this Apr 20, 2023
@pllim pllim deleted the patch-1 branch April 21, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OldJinjaSuffixWarning for alabaster.css_t in unreleased Sphinx 6.2
3 participants