search

sphinx-doc / alabaster

Lightweight, configurable Sphinx theme
http://alabaster.readthedocs.io/
Other
723 stars 185 forks source link

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

Closed pllim closed 1 year ago

pllim commented 1 year ago

To fix #203

pllim commented 1 year ago

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!

jayaddison commented 1 year ago

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: https://github.com/sphinx-doc/sphinx/pull/11165#issuecomment-1509862003 - please let me know if I can explain more.

pllim commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

A