Add THEME_TEMPLATE_OVERRIDES. Refs #2021 #2072

pedrohdz · 2016-12-25T11:11:04Z

Allow for overriding individual templates from the theme by configuring
the Jinja2 Environment loader to search for templates in the
THEME_TEMPLATE_OVERRIDES path before the theme's templates/
directory.

pedrohdz · 2016-12-25T11:14:53Z

Continuation of #2026. I slipped up and closed that PR accidentally. Ties in with getpelican/pelican-themes#466.

avaris · 2016-12-26T19:28:14Z

Doesn't this make EXTRA_TEMPLATES_PATHS obsolete?

Also, IMO, the docs for this should go to the Themes subsection, rather than Template pages.

pedrohdz · 2016-12-27T03:25:39Z

@avaris,

Doesn't this make EXTRA_TEMPLATES_PATHS obsolete?

Might deprecate EXTRA_TEMPLATES_PATHS. I did not want to eliminate, or modify EXTRA_TEMPLATES_PATHS since it might mess up existing users. Should we axe EXTRA_TEMPLATES_PATHS in this change or give people time to transition? Maybe there's an argument for keeping both?

Also, IMO, the docs for this should go to the Themes subsection, rather than Template pages.

Maybe? I figured since it is one replacing or enhancing EXTRA_TEMPLATES_PATHS it should be put in the same section. The TEMPLATE in `THEME_TEMPLATE_OVERRIDES was another reason for putting it in the Template pages section.

I'm amenable on either of these.

avaris · 2016-12-28T19:30:59Z

Deprecating EXTRA_TEMPLATES_PATHS here with a warning should be fine. It can be moved to the new THEME_TEMPLATE_OVERRIDES to be backwards compatible. Only issue I see is that, EXTRA_TEMPLATES_PATHS wasn't overriding the existing templates, whereas this one will. However, that wasn't a feature before, so I don't see any reason for people to have 'overriding' templates in EXTRA_TEMPLATES_PATHS since they won't do anything and will be ignored anyway.

The reason I say, doc should go to Themes section is the primary use of this setting. It's to modify an existing theme with adding or replacing templates. Template pages is more about TEMPLATE_PAGES setting which are one-off templates that produce single output. They are more like Page contents. EXTRA_TEMPLATES_PATHS was put on there, I think, because DIRECT_TEMPLATES was there ^[*]. Although, it's also misleading. It says "These templates need to use DIRECT_TEMPLATES setting.", which isn't exactly true. You should be able to use any template in that setting like other theme templates (e.g. custom template for a content using template metadata).

[*]: Why is DIRECT_TEMPLATES there? It's in a grey area. Not exactly a theme setting, at the same time not really a TEMPLATE_PAGES but similar enough. However, conventionally, it's tied to a theme more than it's tied to TEMPLATE_PAGES

pedrohdz · 2016-12-30T02:31:42Z

Noted... I should be able to get to updating the PR next week...

Thanks!

pedrohdz · 2017-01-09T12:20:46Z

All set @avaris.

almet · 2017-04-21T21:56:53Z

This looks all good to me. I'm in favor of merging it. Any other thoughts?

ssteinerx · 2017-04-21T22:04:32Z

@almet Is the discussion of child themes separate from this, or does this PR obsolete that discussion? In my mind a simple override, while useful, is not the same as being to inherit from and extend a parent theme.

pedrohdz · 2017-04-22T03:48:33Z

@ssteinerx,
I totally agree, template overrides is not the same as being to inherit and extend from an existing parent theme. Though THEME_TEMPLATE_OVERRIDES and inheritance are not mutually exclusive either. Some things to note:

This works as a stop gap until Inherit from arbitrary theme #1092 gets figured out. That request has dragged on for 3+ years and some of the original implementation details are being rehashed.
It fixes what I view as a hobbled EXTRA_TEMPLATES_PATHS while maintaining backwards compatibility. There might have been a reason to implement EXTRA_TEMPLATES_PATHS the way it was, but I question it. (I could be wrong here :-) )
Moves us away from having to fork themes and have to deal with merging.
Even if we implement Inherit from arbitrary theme #1092 tomorrow, it still feels like overkill when all that it is needed is replacing a template file or two.
THEME_TEMPLATE_OVERRIDES and inheritance can coexist, when Inherit from arbitrary theme #1092 is implemented. If anything THEME_TEMPLATE_OVERRIDES can leverage inheritance.

THEME_TEMPLATE_OVERRIDES was inspired by how Hugo theme customization works. I tested out Hugo when I was experimenting with various static site generators. Hugo came a close second, but Jinja templating swayed me towards Pelican.

ssteinerx · 2017-04-22T11:12:32Z

@digitalrounin I agree, and vote to merge this to allow overriding parts of a template completely while we workout/bike shed #1092.

almet · 2017-04-22T21:09:59Z

Lets summon @avaris on this one :-) any feedback ?

avaris

Oh, sorry. I thought I had approved this. Apparently I forgot. Good to go.

ingwinlu

I have several issues with this pr as shown in the review comments.

Additionally:

Why the effort of introducing a new setting instead of just moving the existing EXTRA_TEMPLATES_PATHS to where you now introduced _OVERRIDES?
You deprecate EXTRA_TEMPLATES_PATHS but not really because you don't do anything with it. Either move stuff from the old setting to the new one or don't deprecate.

ingwinlu · 2017-04-23T12:50:09Z

pelican/tests/test_generators.py

+    def test_theme_overrides(self):
+        """
+            Test that the THEME_TEMPLATE_OVERRIDES configuration setting is
+            being utilized correctly the Generator.


utilized correctly in the Generator

ingwinlu · 2017-04-23T13:00:44Z

docs/settings.rst

-   A list of paths you want Jinja2 to search for templates.  Can be used to
-   separate templates from the theme.  Example: projects, resume, profile ...
-   These templates need to use ``DIRECT_TEMPLATES`` setting.
+   ``EXTRA_TEMPLATES_PATHS`` is being depricated, use


is there no sphinx/rst directive to better emphasize deprecation?

ingwinlu · 2017-04-23T13:01:42Z

pelican/generators.py

@@ -52,6 +52,7 @@ def __init__(self, context, settings, path, theme, output_path,
        # templates cache
        self._templates = {}
        self._templates_path = []
+        self._templates_path += self.settings['THEME_TEMPLATE_OVERRIDES']


self._templates_path = [] self._templates_path += self.settings['THEME_TEMPLATE_OVERRIDES']

is equal to

self._templates_path = self.settings['THEME_TEMPLATE_OVERRIDES']

ingwinlu · 2017-04-23T13:04:47Z

pelican/settings.py

@@ -97,6 +97,7 @@ def load_source(name, path):
    'RELATIVE_URLS': False,
    'DEFAULT_LANG': 'en',
    'DIRECT_TEMPLATES': ['index', 'tags', 'categories', 'authors', 'archives'],
+    'THEME_TEMPLATE_OVERRIDES': [],


we use theme_templates internally, might want to change it for consistency

avaris

I do have to un-approve this (thanks to @ingwinlu for pointing out).

I don't like the way deprecation handled here.

Either the old setting is gone and replaced with the new one:

remove EXTRA_TEMPLATES_PATHS from the docs.
show a warning if the old one is present in the settings and move the values to the new setting.
it won't be used anymore (i.e. no more self._templates_path += self.settings['EXTRA_TEMPLATES_PATHS']).

or we reuse the old setting with new functionality (as @ingwinlu suggested)

ssteinerx · 2017-04-23T15:50:47Z

My reading is that EXTRA_TEMPLATES_PATHS files have to be used by DIRECT_TEMPLATES while THEME_TEMPLATE_OVERRIDES is intended to allow overriding any template with that from another non-theme directory.

I find this really confusing but I also never used EXTRA_TEMPLATES_PATHS because I didn't really get the point of it.

I thought the point of this PR was to add a setting to allow overriding templates not used in DIRECT_TEMPLATES and, so, is a separate setting.

avaris · 2017-04-24T13:24:40Z

Current doc for EXTRA_TEMPLATES_PATHS is a bit misleading. Templates from EXTRA_TEMPLATES_PATHS can be treated like any other template. You can use it with DIRECT_TEMPLATES, as the docs suggest, but you can also use them for custom templates in Articles or Pages with template metadata.

The only difference between EXTRA_TEMPLATES_PATHS and THEME_TEMPLATE_OVERRIDES (as proposed in this PR) is the position in the lookup chain. EXTRA_TEMPLATES_PATHS was after the theme, so any conflicting templates were resolved from the theme (i.e. no way to override). THEME_TEMPLATE_OVERRIDES comes before the theme, so it has priority in case of conflict.

ssteinerx · 2017-04-24T14:50:54Z

If I'm reading this right, you can set the theme, then use EXTRA_TEMPLATES_PATHS instead of the the proposed PR and it's just a documentation issue.

avaris · 2017-04-24T15:27:32Z

No... EXTRA_TEMPLATES_PATH can't override theme templates (which is the whole purpose of this PR). Current lookup chain is like this:

templates coming from the THEME
EXTRA_TEMPLATES_PATHS
simple built-in theme

Jinja will look through these in this order and return the first matching template it finds. So, with EXTRA_TEMPLATES_PATHS you can't provide your alternate article.html template. Because the one in theme will be found earlier and used always.

This PR adds the new setting to the top of the chain, so if the template that's being searched is there, it'll be used (instead of the one from the theme).

That being said, there is also a documentation issue with EXTRA_TEMPLATES_PATHS currently. That setting does more than what is documented. It's not limited to DIRECT_TEMPLATES. It's basically an alternate template source for all template related functionality.

avaris · 2017-04-24T15:40:57Z

By the way, it might be worth to include the current theme as a PrefixLoader (similar to simple, say with !theme prefix) with this PR. Especially for the small variations of the override. Because with the way the PR is right now, you pretty much need to write the override from scratch (you can't access the theme template because it's being overridden). If the prefixed one is added, you can use the theme version as a base and override relevant blocks:

{% extends '!theme/article.html' %}
{% block someblock %}
My override
{% endblock %}

ssteinerx · 2017-04-24T15:49:02Z

@avaris So, if you put your parent theme in EXTRA_TEMPLATES_PATHS, and your "child template" as the regular theme, you accomplish the same thing with no code changes?

avaris · 2017-04-24T16:00:51Z

A theme is more than just the templates. You'd need to duplicate the static part (CSS, JS, etc) of the "parent theme" in your "child theme" which is no better than copying the theme, modifying whatever you want and using it as THEME.

ssteinerx · 2017-04-24T16:03:51Z

@avaris Yes, I suspected that the other chunks 'wouldn't come along for the ride.' Since I hadn't looked at EXTRA_TEMPLATES_PATHS carefully due to the documented limitation of its use to direct templates I was hoping that this behaviour might have been overlooked, too 😉.

ssteinerx · 2017-04-24T16:45:52Z

I wonder how traumatic it would be to just add inheritance of the CSS/JS etc. from EXTRA_TEMPLATES_PATHS and updating the documentation to reflect the previous undocumented behaviour and the new behaviour at the same time.

My guess is that EXTRA_TEMPLATES_PATHS was probably used very little due to its documented restriction and, since the theme will have its own CSS and JS for anyone using themes in the regular way, this shouldn't cause any problems at all.

I also agree that adding the PrefixLoader ability would make it hard to really come up with more to add for true "child themes."

avaris · 2017-04-24T17:46:37Z

Real theme inheritance is required if the child theme contains its own static content (e.g. customized main CSS). It could still be possible with certain settings config, but would certainly be "hacky". For template-only modifications, this PR should be sufficient.

pedrohdz · 2017-04-25T10:04:01Z

@ingwinlu - Good stuff... Keep it coming... I just submitted a new round of changes. Let me know if I missed, or misinterpreted, anything.

@avaris - Good idea on !theme. I implemented it. Please check it out. I'm stumped on how to implement unit tests for it though. Any ideas?

@ssteinerx - Actually, anyone that wants to add custom pages via DIRECT_TEMPLATES would likely be utilizing EXTRA_TEMPLATES_PATHS.

If someone whats to go in and add their own static content (images, JS, CSS) STATIC_PATHS should be used and then update the corresponding template file, typically through forking a theme, but soon via THEME_TEMPLATES_OVERRIDES as well. There's also CUSTOM_CSS depending on the theme you are using.

In my mind, Pelican users can wear one of two hats:

Content Creator - Blog writers and whatnot.
Theme Maintainer/Creator - Name says it all.

A person can where one, or both. I view settings such as THEME_TEMPLATES_OVERRIDES, STATIC_PATHS, etc. as way of allowing Content Creators to modify the look and feel of their site without being forced into becoming Theme Maintainers. Avoiding the whole theme forking thing.

THEME_TEMPLATES_OVERRIDES is not meant to be used theme inheritance.

almet · 2017-04-25T20:55:28Z

I love where this is going. Simplicity matters :-)

pedrohdz · 2017-06-11T04:13:14Z

@almet, @ingwinlu, @avaris, @ssteinerx,
Any chance of wrapping this up? Anything else need changing?

avaris · 2017-06-11T15:58:04Z

@digitalrounin for testing it, you can create an override, create appropriate settings, create a generator and use its get_template for your override, then render it and check.

Allow for overriding individual templates from the theme by configuring the Jinja2 `Environment` loader to search for templates in the `THEME_TEMPLATES_OVERRIDES` path before the theme's `templates/` directory.

pedrohdz · 2017-10-10T06:36:43Z

Finally got some free time to work on this!

@avaris,
Tests for !theme prefix added. Added tests for !simple and bad prefixes as well.

@almet,
Any chance we can finally merge this? :-)

justinmayer · 2018-01-07T19:25:30Z

Any last thoughts from @getpelican/reviewers? Your input would be greatly appreciated!

astrojuanlu · 2018-03-01T10:57:55Z

Any chance to move this forward?

justinmayer · 2018-03-01T15:56:32Z

@avaris: Any chance you could take another look and see if anything else is required before merging?

avaris

Sorry for the delay. Looks good for me 👍

justinmayer · 2018-03-05T15:50:03Z

Many thanks to @digitalrounin for the contribution and to everyone who helped review it!

pedrohdz · 2018-03-05T20:05:03Z

Thanks Everyone!

pedrohdz changed the title ~~Add THEME_TEMPLATE_OVERRIDES. Refs 2021~~ Add THEME_TEMPLATE_OVERRIDES. Refs 2021 (Take 2) Dec 25, 2016

pedrohdz mentioned this pull request Dec 25, 2016

pelican-bootstrap3: refactored sidebar (take 2) getpelican/pelican-themes#466

Merged

pedrohdz mentioned this pull request Apr 20, 2017

Inherit from arbitrary theme #1092

Closed

avaris approved these changes Apr 23, 2017

View reviewed changes

ingwinlu suggested changes Apr 23, 2017

View reviewed changes

avaris requested changes Apr 23, 2017

View reviewed changes

Add THEME_TEMPLATE_OVERRIDES. Refs 2021

50af2ed

Allow for overriding individual templates from the theme by configuring the Jinja2 `Environment` loader to search for templates in the `THEME_TEMPLATES_OVERRIDES` path before the theme's `templates/` directory.

quobit mentioned this pull request Mar 1, 2018

Traducir el footer python-spain/web#23

Closed

avaris approved these changes Mar 5, 2018

View reviewed changes

justinmayer added this to the 3.8.0 milestone Mar 5, 2018

justinmayer changed the title ~~Add THEME_TEMPLATE_OVERRIDES. Refs 2021 (Take 2)~~ Add THEME_TEMPLATE_OVERRIDES. Refs #2021 Mar 5, 2018

justinmayer merged commit 72756a5 into getpelican:master Mar 5, 2018

pedrohdz deleted the overrides_ref2021+merge_update branch April 2, 2018 09:39

boxydog mentioned this pull request Oct 28, 2023

Add Multi-theme support #1327

Closed

Add THEME_TEMPLATE_OVERRIDES. Refs #2021 #2072

Add THEME_TEMPLATE_OVERRIDES. Refs #2021 #2072

Conversation

pedrohdz commented Dec 25, 2016

pedrohdz commented Dec 25, 2016

avaris commented Dec 26, 2016

pedrohdz commented Dec 27, 2016

avaris commented Dec 28, 2016 • edited

pedrohdz commented Dec 30, 2016

pedrohdz commented Jan 9, 2017

almet commented Apr 21, 2017

ssteinerx commented Apr 21, 2017

pedrohdz commented Apr 22, 2017

ssteinerx commented Apr 22, 2017

almet commented Apr 22, 2017

avaris left a comment

Choose a reason for hiding this comment

ingwinlu left a comment

Choose a reason for hiding this comment

ingwinlu Apr 23, 2017

Choose a reason for hiding this comment

ingwinlu Apr 23, 2017

Choose a reason for hiding this comment

ingwinlu Apr 23, 2017

Choose a reason for hiding this comment

ingwinlu Apr 23, 2017

Choose a reason for hiding this comment

avaris left a comment • edited

Choose a reason for hiding this comment

ssteinerx commented Apr 23, 2017

avaris commented Apr 24, 2017

ssteinerx commented Apr 24, 2017

avaris commented Apr 24, 2017 • edited

avaris commented Apr 24, 2017 • edited

ssteinerx commented Apr 24, 2017

avaris commented Apr 24, 2017

ssteinerx commented Apr 24, 2017

ssteinerx commented Apr 24, 2017 • edited

avaris commented Apr 24, 2017

pedrohdz commented Apr 25, 2017

almet commented Apr 25, 2017

pedrohdz commented Jun 11, 2017

avaris commented Jun 11, 2017

pedrohdz commented Oct 10, 2017

justinmayer commented Jan 7, 2018

astrojuanlu commented Mar 1, 2018

justinmayer commented Mar 1, 2018

avaris left a comment

Choose a reason for hiding this comment

justinmayer commented Mar 5, 2018

pedrohdz commented Mar 5, 2018

avaris commented Dec 28, 2016 •

edited

avaris left a comment •

edited

avaris commented Apr 24, 2017 •

edited

avaris commented Apr 24, 2017 •

edited

ssteinerx commented Apr 24, 2017 •

edited