Template rework #144

nabobalis · 2022-07-19T21:46:18Z

Trying to fix the templates and tidy them up.

Fixes #143
Fixes #125
Fixes #108

TODO

Update release page for 0.11

nabobalis · 2022-07-19T23:47:57Z

ablog/__init__.py


    return {"version": __version__}  # identifies the version of our extension


 def config_inited(app, config):
-    app.config.templates_path.append(get_html_templates_path())
+    app.config.templates_path = [get_html_templates_path()] + app.config.templates_path


This should prevent ablog being last on the list?

This looks... wrong. This puts ablog before the user-configured templates, which means even the user can't override these files. The paths from this configuration option are placed before theme templates, so as long as ablog's templates path is listed here, themes still won't be able to override the newly moved files.

Ok, so I don't want to add ablog templates to this at all?

I think we do -- just not here. I've been procrastinating looking at this, but I'll take a closer look today.

Ok, so here's my suggestions for what to change this to, to enable basically
everything we've discussed in #143 and #125:

Add a ablog_skip_injecting_base_ablog_templates configuration option, defaulting to False (rename to whatever)

Change this handler to listen to builder-inited instead of config-inited.

Change this line to be:

from sphinx.jinja2glue import SphinxFileSystemLoader if not app.config.ablog_skip_injecting_base_ablog_templates: if not isinstance(app.templates, BuiltinTemplateLoader): raise Exception( "Ablog does not know how to inject templates into with custom " "template bridges. You can use `ablog.get_html_templates_path()` to " "get the path to add in your custom template bridge and set " "`ablog_skip_injecting_base_ablog_templates = False` in your " "`conf.py` file." ) if get_html_templates_path() in app.config.templates_path: raise Exception( "Found the path from `ablog.get_html_templates_path()` in the " "`templates_path` variable from `conf.py`. Doing so interferes " "with Ablog's ability to stay compatible with Sphinx themes that " "support it out of the box. Please remove `get_html_templates_path` " "from `templates_path` in your `conf.py` to resolve this." ) theme = app.builder.theme loaders = app.templates.loaders templatepathlen = app.templates.templatepathlen if theme.get_config("ablog", "inject_templates_after_theme", False): # Inject *after* the user templates and the theme templates, # allowing themes to override the templates provided by this # extension while those templates still serve as a fallback. loaders.append(SphinxFileSystemLoader(get_html_templates_path()) else: # Inject *after* the user templates and *before* the theme # templates. This enables ablog to provide support for themes # that don't support it out-of-the-box, like alabaster. loaders.insert(templatepathlen, SphinxFileSystemLoader(get_html_templates_path())

This code has some fairly useful properties and behaviours:

All the attributes being used here are public, so Sphinx's backwards compatibility policy applies on them, giving us ample time to make changes if Sphinx changes things (deprecation warnings for 2 major versions before removal, which should be more than an year to adapt this code)

This puts the ablog templates after the theme templates, by default, with an escape hatch for users who don't want that. This does depend on them not setting html_template_bridge to something super custom -- that's a very small set of users AFAICT and the error message should give such users all the information that they need to act on it.

This allows themes to change where ablog's template injection occurs, by setting a flag in their theme.conf file:

[ablog] inject_templates_after_theme = true

This would enable themes like Furo to contain their own ablog/catalog.html for example.

This prevents users from fiddling with the ordering of the templates, by not permitting them to inject the Ablog-provided templates manually.

In the spirit of transparency, I've not tested this code in any way -- and written most of this in a Markdown editor -- so no promises on whether this will work. That said, this will enable basically everything we discussed in #143 and #125 (as long as you move the templates, as has been done already). This should also be a drop-in replacement for users who don't set up custom templates or overrides on top of ablog itself. If they do, they might need to move those paths to match the namespace.

One template that you do not want to move is page.html. That's the template that affects the actual Sphinx templating, since Sphinx renders for (almost) every HTML page and we want to override that for alabaster (the default theme).

Thanks for all of this, still trying to work out the kinks but your code was really useful!
I would have never got this working without it.

nabobalis · 2022-07-19T23:48:17Z

ablog/start.py

@@ -216,7 +216,7 @@ def w(t, ls=80):
 ]

 # Add any paths that contain templates here, relative to this directory.
-templates_path = ["{dot}templates", ablog.get_html_templates_path()]
+templates_path = [ablog.get_html_templates_path(), "{dot}templates"]


Same here, should be first.

Users shouldn't need to do this, given that the event handler adds this path already for them.

Right, I will remove this from the setup script.

Actually, we might want to add a check to reject configurations that do this. If users are doing this, they'll be able to break the way that the suggestion I've made for handling #143 and #125 works in a manner that's gonna be difficult for the user to diagnose (#144 (comment)).

To elaborate on what I mean:

Furo adds support for Ablog and declares inject_templates_after_theme = true.

The theme's templates would take precedence over Ablog's templates

This allows Furo to have a custom page.html and ablog/catalog.html, that are different from Ablog's.

The above suggested code will make Ablog inject its own templates after the theme templates.

If the user does nothing more, everything will work just fine.

If the user sets this in their conf.py file, things will break.

The Ablog templates will also be added before the theme templates (since user templates are injected before theme templates).

Ablog's own templates would take precedence over the theme's templates.

See the first bullet in the previous list -- that contradicts with this, and the user will see the wrong behaviour (i.e. the one noted in this list).

The most annoying part of this for the user is that the build won't fail, since the templates will still likely render correctly -- they'll be getting picked up from the wrong place and cause improper markup to be generated in the final rendered templates!

pradyunsg · 2022-07-22T08:36:02Z

FWIW, one template that we do not want to move is page.html. That's the template that affects the actual Sphinx rendering, since Sphinx renders for (almost) every HTML page and we want to override that for alabaster (the default theme).

ablog/__init__.py

pradyunsg · 2022-08-05T08:38:23Z

ablog/__init__.py

+    if not app.config.inject_templates_after_theme:
+        if not isinstance(app.builder.templates, BuiltinTemplateLoader):
+            raise Exception(
+                "Ablog does not know how to inject templates into with custom "
+                "template bridges. You can use `ablog.get_html_templates_path()` to "
+                "get the path to add in your custom template bridge and set "
+                "`inject_templates_after_theme = False` in your "
+                "`conf.py` file."
+            )
+        if get_html_templates_path() in app.config.templates_path:
+            raise Exception(
+                "Found the path from `ablog.get_html_templates_path()` in the "
+                "`templates_path` variable from `conf.py`. Doing so interferes "
+                "with Ablog's ability to stay compatible with Sphinx themes that "
+                "support it out of the box. Please remove `get_html_templates_path` "
+                "from `templates_path` in your `conf.py` to resolve this."
+            )


Hmm... this option seems to be doing the wrong thing.

I'd suggest changing this to skip_injecting_base_ablog_templates and changing the conf.py documentation related to this as well (I'll make a separate suggestion for that).

The goal isn't to skip doing any checks when that is set, but to avoid doing anything when that is set. This would basically be for users who are using a template bridge that does not work like Sphinx's own template bridge does. This logic is highly coupled with the way Sphinx's BuiltinTemplateLoader works and we can't know if we'd present esoteric errors with alternatives -- and that's why we have the isinstance(..., BuiltinTemplateLoader) check.

Suggested change

if not app.config.inject_templates_after_theme:

if not isinstance(app.builder.templates, BuiltinTemplateLoader):

raise Exception(

"Ablog does not know how to inject templates into with custom "

"template bridges. You can use `ablog.get_html_templates_path()` to "

"get the path to add in your custom template bridge and set "

"`inject_templates_after_theme = False` in your "

"`conf.py` file."

)

if get_html_templates_path() in app.config.templates_path:

raise Exception(

"Found the path from `ablog.get_html_templates_path()` in the "

"`templates_path` variable from `conf.py`. Doing so interferes "

"with Ablog's ability to stay compatible with Sphinx themes that "

"support it out of the box. Please remove `get_html_templates_path` "

"from `templates_path` in your `conf.py` to resolve this."

)

if app.config.skip_injecting_base_ablog_templates:

return

if not isinstance(app.builder.templates, BuiltinTemplateLoader):

raise Exception(

"Ablog does not know how to inject templates into with custom "

"template bridges. You can use `ablog.get_html_templates_path()` to "

"get the path to add in your custom template bridge and set "

"`skip_injecting_base_ablog_templates = False` in your "

"`conf.py` file."

)

if get_html_templates_path() in app.config.templates_path:

raise Exception(

"Found the path from `ablog.get_html_templates_path()` in the "

"`templates_path` variable from `conf.py`. Doing so interferes "

"with Ablog's ability to stay compatible with Sphinx themes that "

"support it out of the box. Please remove `get_html_templates_path` "

"from `templates_path` in your `conf.py` to resolve this."

)

Oh wait, this can't be an early return since this function is also doing blog_post_pattern stuff.

Well, the idea still stands, although the specific code suggestion here is wrong. We'd probably want to indent the lines that add the template instead, and only inject templates if the user hasn't explicitly configured a skip.

It might make sense to make the two of them smaller functions, which would change this function to:

def builder_inited(app): if not app.config.skip_injecting_base_ablog_templates: _inject_base_templates(app) _inject_matched_blog_posts(app)

I moved the blog_post_pattern block back to a config-initted function where it used to be. This should separate the two parts nicely.

ablog/blog.py

ablog/start.py

pradyunsg · 2022-08-05T08:53:24Z

So... I did some preliminary testing, starting with ablog start from the main branch, building with that branch, then switching to this branch and making changes until things build, and finally switching to Furo to see how things build. Notes below.

Everything with main worked correctly -- ablog start followed by sphinx-build. :)

note 1: The ablog start's conf.py template should get the same treatment as sphinx-doc/sphinx#10056, as well as a cleanup of the stuff it's got that's redundant/useless today (alabaster doesn't need to be imported, os and sys are unused, generated code isn't black-ready etc). We may also want to switch to using sphinx-quickstart's templating logic directly, instead of doing .format on a hard-coded string in a Python file -- I'll file a PR for this sometime soon.

On the first build with the new ablog, users will get the following error:

loading pickled environment... failed
failed: No such config value: inject_templates_after_theme

Extension error (ablog):
Handler <function builder_inited at 0x104675c10> for event 'builder-inited' threw an exception (exception: Found the path from `ablog.get_html_templates_path()` in the `templates_path` variable from `conf.py`. Doing so interferes with Ablog's ability to stay compatible with Sphinx themes that support it out of the box. Please remove `get_html_templates_path` from `templates_path` in your `conf.py` to resolve this.)

I really like this error message! It provides clear context on what went wrong, why that's a problem and what the user can do. We don't have control over the specific presentation out of Sphinx and that would be my only complaint about this.

However, once I resolved that by following the instructions, you get the following error:

Theme error:
An error happened in rendering the page about.
Reason: TemplateNotFound('postcard.html')

This is coming out of the html_sidebars declaration, and is a very opaque error message. We should improve this.

note 2: We should write a "migrating from {old} ablog to {new} ablog", as a blog post. I'd be happy to write this. :)
note 3: I think it's sensible to add back the "old" files, with the following content:

{{ warning("foo.html is an old template path, that is no longer used by ablog. Please use ablog/foo.html instead.") }}

This would mean that users see a warning from Sphinx, that provides guidance, instead of the opaque error above.

Changing the theme to Furo hits https://github.com/pradyunsg/furo/blob/main/src/furo/theme/furo/layout.html. After I added the following to Furo's theme.conf (and removed alabaster-specific files from html_sidebars), things rendered with Furo's page.html template! 🎉

[ablog]
inject_templates_after_theme = true

This means that it is definitely possible now, to add first-class support for ABlog in Furo and other themes! Right now, quite a few things don't render correctly with Furo and the default templates from ABlog, but that's expected and something for future me to deal with. Right now, I'm very excited! :)

nabobalis · 2022-08-07T01:56:46Z

note 1: The ablog start's conf.py template should get the same treatment as sphinx-doc/sphinx#10056, as well as a cleanup of the stuff it's got that's redundant/useless today (alabaster doesn't need to be imported, os and sys are unused, generated code isn't black-ready etc). We may also want to switch to using sphinx-quickstart's templating logic directly, instead of doing .format on a hard-coded string in a Python file -- I'll file a PR for this sometime soon.

Yes that part of ablog is pretty old now, it would be great to improve this.

note 2: We should write a "migrating from {old} ablog to {new} ablog", as a blog post. I'd be happy to write this. :)

That would be incredible!
It is something that would need to exist before a release.
I have created a 0.10 branch and plan add multiple versions to RTD so people can check the old version for a while.

note 3: I think it's sensible to add back the "old" files, with the following content:
{{ warning("foo.html is an old template path, that is no longer used by ablog. Please use ablog/foo.html instead.") }}

I will do that in this PR as well but I use the updated html files from this PR.

Changing the theme to Furo hits pradyunsg/furo@main/src/furo/theme/furo/layout.html. After I added the following to Furo's theme.conf (and removed alabaster-specific files from html_sidebars), things rendered with Furo's page.html template! 🎉
[ablog]
inject_templates_after_theme = true
This means that it is definitely possible now, to add first-class support for ABlog in Furo and other themes! Right now, quite a few things don't render correctly with Furo and the default templates from ABlog, but that's expected and something for future me to deal with. Right now, I'm very excited! :)

That is brilliant, I want to thank you for your help again on this PR.

ablog/__init__.py

pradyunsg · 2022-08-15T17:56:32Z

@nabobalis Anything I can do to help move this forward?

nabobalis · 2022-08-15T17:58:46Z

@nabobalis Anything I can do to help move this forward?

I am not sure, I forgot about this and got the wrong import which is why the CI is broken.

But I think in theory its complete. So maybe just a glance over the Python changes would be ideal to make sure its all sane.

nabobalis · 2022-08-16T05:41:00Z

ablog/__init__.py

@@ -6,6 +6,9 @@
 from glob import glob
 from pathlib import PurePath

+from sphinx.builders.html import StandaloneHTMLBuilder


I think this is the correct HTMLBuilder.

nabobalis · 2022-08-16T05:54:39Z

Other than the need to fix the wrong setup for the dev build. I think everything is in place.

pradyunsg

LGTM, based on a desk skim.

nabobalis · 2022-08-17T15:56:46Z

LGTM, based on a desk skim.

Thanks again. I will branch off main to keep a 0.10 branch around for a bit and then merge this.

choldgraf · 2022-08-18T07:40:45Z

🎉

bowentan · 2022-11-21T06:30:32Z

Hi, I still got the error for layout.html when using the latest ablog with furo theme. Is the problem unresolved? Or do I need additional works on configurations?

nabobalis · 2022-11-21T06:47:36Z

Hi, I still got the error for layout.html when using the latest ablog with furo theme. Is the problem unresolved? Or do I need additional works on configurations?

Hello, this patch never made it to a release version at the moment.

My plan was to ship this in 0.11 but I got distracted and have not done a release of that.

bowentan · 2022-11-21T07:14:03Z

Hello, this patch never made it to a release version at the moment.

My plan was to ship this in 0.11 but I got distracted and have not done a release of that.

Oh i see.. Thanks!

Jaharmi · 2022-12-14T21:35:15Z

I am also seeing this on a new site / test setup I'm working on. I had ablog 0.10.30 installed and made sure it was updated to 0.10.33. It looks like there is no 0.11 release yet.

nabobalis · 2022-12-14T21:37:03Z

Yes unfortunately that is the case. Real life has got in my way recently and that will not change for me until Christmas.

nabobalis · 2022-12-29T01:49:56Z

Sorry it has taken me this long but I released ablog 0.11.0rc1.

I did not want to release such a large change without at least a brief RC period.

So if those who wanted to try this out for me, that would be great.

I plan to test some of the projects I maintain that use ablog as well but its a very limited test.

choldgraf · 2022-12-29T17:32:35Z

Nice - sounds good I will give it a shot and open issues

nabobalis force-pushed the tweaks branch 5 times, most recently from ca270e4 to 82f817c Compare July 19, 2022 23:34

nabobalis mentioned this pull request Jul 19, 2022

Make it possible for Sphinx themes, to provide their own templates for ablog to use! #125

Closed

nabobalis marked this pull request as ready for review July 19, 2022 23:47

nabobalis commented Jul 19, 2022

View reviewed changes

nabobalis force-pushed the tweaks branch from 4ffd065 to 9300c8f Compare August 3, 2022 05:03

nabobalis marked this pull request as draft August 3, 2022 05:03

Templates rework

0f9753c

nabobalis force-pushed the tweaks branch from 9300c8f to 0f9753c Compare August 3, 2022 05:14

drop older versions of packages

a0d0e1c

nabobalis force-pushed the tweaks branch 6 times, most recently from 04161d4 to e9b552c Compare August 3, 2022 05:46

pradyunsg reviewed Aug 3, 2022

View reviewed changes

ablog/__init__.py Outdated Show resolved Hide resolved

nabobalis force-pushed the tweaks branch 3 times, most recently from ab76901 to ad23e98 Compare August 5, 2022 05:25

Extra changes to get building again

e04b7b6

nabobalis force-pushed the tweaks branch from ad23e98 to e04b7b6 Compare August 5, 2022 05:28

nabobalis marked this pull request as ready for review August 5, 2022 05:33

pradyunsg reviewed Aug 5, 2022

View reviewed changes

split up back to config and builder parts

934017c

nabobalis force-pushed the tweaks branch from a87cb65 to 934017c Compare August 7, 2022 01:47

add back latex tests

66f81bb

nabobalis force-pushed the tweaks branch from 55f8028 to 66f81bb Compare August 7, 2022 01:50

nabobalis force-pushed the tweaks branch from 6356950 to 872bf66 Compare August 7, 2022 02:27

added back old templates but with warning

9abb8c4

nabobalis force-pushed the tweaks branch from 872bf66 to 9abb8c4 Compare August 7, 2022 02:28

pradyunsg reviewed Aug 7, 2022

View reviewed changes

ablog/__init__.py Outdated Show resolved Hide resolved

more general catch

f22aff3

update import

323890f

nabobalis commented Aug 16, 2022

View reviewed changes

nabobalis requested a review from pradyunsg August 17, 2022 03:40

pradyunsg approved these changes Aug 17, 2022

View reviewed changes

nabobalis merged commit 6626ad6 into main Aug 18, 2022

nabobalis deleted the tweaks branch August 18, 2022 02:25

This was referenced Jan 2, 2024

Tags in tag cloud has same font-size #267

Closed

Add stylesheet for tagcloud #268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template rework #144

Template rework #144

nabobalis commented Jul 19, 2022 •

edited

nabobalis Jul 19, 2022

pradyunsg Jul 20, 2022 •

edited

nabobalis Jul 20, 2022

pradyunsg Jul 22, 2022

pradyunsg Jul 22, 2022 •

edited

nabobalis Aug 3, 2022 •

edited

nabobalis Jul 19, 2022

pradyunsg Jul 20, 2022 •

edited

nabobalis Jul 20, 2022

pradyunsg Jul 22, 2022

pradyunsg Jul 22, 2022 •

edited

pradyunsg commented Jul 22, 2022 •

edited

pradyunsg Aug 5, 2022 •

edited

pradyunsg Aug 5, 2022

pradyunsg Aug 5, 2022 •

edited

nabobalis Aug 7, 2022 •

edited

pradyunsg commented Aug 5, 2022

nabobalis commented Aug 7, 2022

pradyunsg commented Aug 15, 2022

nabobalis commented Aug 15, 2022 •

edited

nabobalis Aug 16, 2022

nabobalis commented Aug 16, 2022

pradyunsg left a comment

nabobalis commented Aug 17, 2022

choldgraf commented Aug 18, 2022

bowentan commented Nov 21, 2022

nabobalis commented Nov 21, 2022

bowentan commented Nov 21, 2022

Jaharmi commented Dec 14, 2022 •

edited

nabobalis commented Dec 14, 2022

nabobalis commented Dec 29, 2022

choldgraf commented Dec 29, 2022

Template rework #144

Template rework #144

Conversation

nabobalis commented Jul 19, 2022 • edited

Choose a reason for hiding this comment

pradyunsg Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Jul 22, 2022 • edited

Choose a reason for hiding this comment

nabobalis Aug 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Jul 22, 2022 • edited

Choose a reason for hiding this comment

pradyunsg commented Jul 22, 2022 • edited

pradyunsg Aug 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Aug 5, 2022 • edited

Choose a reason for hiding this comment

nabobalis Aug 7, 2022 • edited

Choose a reason for hiding this comment

pradyunsg commented Aug 5, 2022

nabobalis commented Aug 7, 2022

pradyunsg commented Aug 15, 2022

nabobalis commented Aug 15, 2022 • edited

Choose a reason for hiding this comment

nabobalis commented Aug 16, 2022

pradyunsg left a comment

Choose a reason for hiding this comment

nabobalis commented Aug 17, 2022

choldgraf commented Aug 18, 2022

bowentan commented Nov 21, 2022

nabobalis commented Nov 21, 2022

bowentan commented Nov 21, 2022

Jaharmi commented Dec 14, 2022 • edited

nabobalis commented Dec 14, 2022

nabobalis commented Dec 29, 2022

choldgraf commented Dec 29, 2022

nabobalis commented Jul 19, 2022 •

edited

pradyunsg Jul 20, 2022 •

edited

pradyunsg Jul 22, 2022 •

edited

nabobalis Aug 3, 2022 •

edited

pradyunsg Jul 20, 2022 •

edited

pradyunsg Jul 22, 2022 •

edited

pradyunsg commented Jul 22, 2022 •

edited

pradyunsg Aug 5, 2022 •

edited

pradyunsg Aug 5, 2022 •

edited

nabobalis Aug 7, 2022 •

edited

nabobalis commented Aug 15, 2022 •

edited

Jaharmi commented Dec 14, 2022 •

edited