Normalize formatting of sample theme Jinja templates #2379

strburst · 2018-07-08T00:47:58Z

Description

This PR normalizes the formatting of the Jinja templates in the simple and notmyidea themes, making them easier to read and indentation rules clearer for future contributors. The diff is huge, but the changes are entirely mechanical/whitespace-related. As far as I'm aware, only the following changes are applied.

4-space indentation (there was a small amount of 2-space indentation). No "exceptions" like content inside <body> or <section>.
Conform to the conventions that are used in the examples in the official Jinja template documentation. I have listed the rules as I understand them in the commit message of 59750b3.

Multiline nested tags like

<tag1><tag2>
    ... nested content ...
</tag2></tag1>

are broken up into

<tag1>
    <tag2>
        ... nested content ...
    </tag2>
</tag1>

but nested tags on one line like

<li><a href="...">...</a></li>

are untouched/allowed.

Other stuff like CSS is untouched.

To maintain your sanity, I would recommend focusing on just the templates in the themes themselves (grouped together at the bottom of the diffs and also separated into one commit for each theme) and not the html output, then doing a pass with git diff -w or checking "Hide whitepsace changes" on the Github diff to ignore whitespace changes (which will only show changes like rule 3).

Note that this doesn't make the output html pretty at all due to the fact that nested Jinja templates are not aware of indentation level at all.

Testing

I tested this by generating the output for both the simple and notmyidea themes and running the diff tests, then also inspecting the output in a browser to make sure everything looks the same.

Implementation note

Unfortunately, there doesn't seem to be a good standalone Jinja template formatter (the most recommended option I found when searching was atom-beautify, which is part of Atom, of course). I used the tool htmlbeautifier, but had to apply a lot of Jinja-specific rules manually on top.

strburst · 2018-07-14T00:16:05Z

If this PR isn't isomorphic, I will eat my own hat (metaphorically). 🎩

avaris

Not my favorite type of review 😕 . I briefly look over it and looks OK. I guess...

justinmayer · 2018-11-12T07:24:59Z

Hi Allen. Many thanks for your formatting improvements to the Jinja templates in the simple and notmyidea themes. There was one PR ahead of yours in the queue (#2374), and unfortunately it also touched some of the same files. Would you be so kind as to rebase on current master, resolve any conflicts, and re-form your commit(s)?

My apologies for the extra work. My goal is to release Pelican 4.0 within the next 24 hours, so if you could update your PR within that time frame, we should be able to include it in the release. 😅

I used the tool [htmlbeautify][1] like so htmlbeautifier -t 4 -b 1 pelican/themes/simple/templates/*.html htmlbeautifier -t 4 -b 1 pelican/themes/notmyidea/templates/*.html and then manually adjusted the output. The final result follows the following rules. - 4-space indentation - Multiline nested tags like <tag1><tag2> ... nested content ... </tag2></tag1> are broken up into <tag1> <tag2> ... nested content ... </tag2> </tag1> but if everything is on one like this <tag1><tag2>... nested content ...</tag2></tag1> it's OK. - Treat new blocks like tags when indenting. E.g. {% if cond %} <p>hello</p> {% endif %} However, observe the following exceptions: - Don't indent inside a {% block %}, {% macro %}, or if the block takes up the entire file. - If a {% for ... %} block takes up the entire content of an HTML tag, don't indent it. E.g. <ul id="navigation"> {% for item in navigation %} <li><a href="{{ item.href }}">{{ item.caption }}</a></li> {% endfor %} </ul> I tried to follow the examples in the official Jinja documentation as closely as possible: <http://jinja.pocoo.org/docs/2.10/templates/>. Used `git diff --check` to perform basic whitespace checks. [1]: [https://github.com/threedaymonk/htmlbeautifier]

strburst · 2018-11-14T05:19:44Z

Hi @justinmayer, sorry for the delay; I've rebased everything as requested. Additionally, I also took a closer look at the official Jinja template documentation and realized there were some indentation rules I didn't see before, so I've fixed this. I put a complete list of indentation rules in the commit message of 59750b3.

mosra · 2018-11-16T10:32:54Z

pelican/tests/output/basic/a-markdown-powered-article.html

+            <h1><a href="/">A Pelican Blog </a></h1>
+            <nav>
+                <ul>
+                            <li><a href="/tag/oh.html">Oh Oh Oh</a></li>


Suggested change

<li><a href="/tag/oh.html">Oh Oh Oh</a></li>

<li><a href="/tag/oh.html">Oh Oh Oh</a></li>

If we're going for an OCD-correct indentation, this list should go four spaces back I think ;)

mosra · 2018-11-16T10:33:58Z

pelican/tests/output/basic/a-markdown-powered-article.html


-</footer><!-- /.post-info -->      <p>You're mutually oblivious.</p>
+</footer><!-- /.post-info -->            <p>You're mutually oblivious.</p>


Maybe a newline should go here as well? Not sure about the indent tho.

Suggested change

</footer> <p>You're mutually oblivious.</p>

</footer>

<p>You're mutually oblivious.</p>

mosra · 2018-11-16T10:52:11Z

(Disclaimer: I'm an OCD freak when it comes to formatting. Take with a grain of salt 😆 )

In my themes I'm not indenting the <head> / <body> tags to avoid excessive indentation. Additionally for markup I'm personally using only 2 spaces so it then ends up looking like this. (Just an opinion, not demanding to use two-space indentation here as well ;).)

The indentation mostly makes sense, except for the two cases I mentioned above. The generated output still has quite a lot of (what seem to be rather random) empty lines, not sure what to do about that. Maybe remove them all? Some things coming from {% block %} also seem to be not indented -- there's the indent() Jinja2 filter that could help with that, but note that the indentation affects content of <pre> tags, so better not do that for the main page content.

My thinking is that if we're going to do such a huge change in the formatting, we might as well finish it to the extreme to avoid doing similar work again in the future. (Because this always results in many coflicts for everyone who's modifying the themes as well.) It also makes testing easier, since with clean markup it's clearer to see what a particular change did to the theme.

strburst · 2018-11-20T16:59:05Z

Hi @mosra, thanks for your comments! It makes me feel like someone cares about all the indentation work I had to do, haha.

First, it's important to note that we basically can't edit the generated output directly; it has to exactly match what is generated, because there are some tests that assert this (there's a little more info here). So sadly, I can't apply any of your suggested changes to pelican/tests/output/*. Looking at the templates that generated them, they do seem to be pretty reasonable already, but Jinja seems to like to join sub-blocks to the previous line in certain circumstances.

Second, it would be nice to do, but I have pretty much given up trying to make the HTML output look pretty; last time I looked at it, it seems like Jinja doesn't have a concept of indenting content substituted in from blocks and includes. For example,

{# template_parent.html #}
<tag1>
    <tag2>
        <tag3>
            <p>From parent</p>
            {% block content %}
            {% endblock %}
        </tag3>
    </tag2>
</tag1>

{# template_child.html #}
{% extends template_parent.html %}
{% block content %}
Hi everyone!
{% endblock %}

<!-- Output -->
<tag1>
    <tag2>
        <tag3>
            <p>From parent</p>
Hi everyone!
        </tag3>
    </tag2>
</tag1>

Note that the child template is basically just substituted in line-wise, with no awareness of indentation. And of course, it would be a Bad Thing to couple child templates to how much indentation is in their parents. Hopefully, people will be looking at the templates that generated the output much more often rather than the output itself.

But if there were some sort of setting that adds the same indentation that the template was on into the file, that might be really nice. We could also run it through a prettifier/minifier, but that is only something the assets plugin can do, and not native Pelican.

I believe changing indentation from 4 spaces to 2 spaces would actually be fairly simple; it'll just a sed command. I do think 2 spaces is more trendy lately in web development (especially Javascript); I only chose 4 spaces because the Jinja doc examples use it. So I could be easily convinced either way.

mosra · 2018-11-26T11:04:44Z

First, it's important to note that we basically can't edit the generated output directly;

Of course ;) To make myself clear, I was looking at the output to see what's wrong in the templates (and so all my suggestions are meant to be "this output looks wrong and the source template should be fixed").

We could also run it through a prettifier/minifier

No, please not, that's a one-way ticket to hell. I know too many projects that started applying minifiers to their test results instead of fixing the root causes and it went only downhill from there.

Jinja seems to like to join sub-blocks to the previous line in certain circumstances.

This can be ~~fixed~~ worked around by putting an explicit empty line in the template (yes, I know, it's less than ideal). Sometimes also things like {%+ or +%} help to preserve the space before / after. You have to try though, I don't have a guide that would work 100%.

For the blocks I'm afraid I don't have a solution, I was always explicitly indenting the content of {% block %}. I don't think it would be a Bad Thing, since it's always clear how much the child block needs to be indented. Also, again, you shouldn't indent the main content because it will break <pre> tags. I'm doing something like this in that case, to denote where the unindented content is pasted:

<tag1>
    <tag2>
        <tag3>
            <p>From parent</p>
<!-- content -->
{% block content %}
{% endblock %}
<!-- /content -->
        </tag3>
    </tag2>
</tag1>

For includes (which can happen at multiple places and so the indentation is not always clear), I have a workaround -- been using this myself on a few places:

    <footer class="m-container">
      <div class="m-row">
        <div class="m-col-m-10 m-push-m-1 m-nopadb">
          {% macro footer() %}{% include "article_footer.html" %}{% endmacro %}
          {{ footer()|indent(10) }}
        …

kdeldycke · 2020-09-03T08:48:19Z

For this kind of heavy formatting, I propose to automate that task to lower the burden of maintenance.

Here is for example how I enforce Jinja and HTML format on my Plumage theme for Pelican: https://github.com/kdeldycke/plumage/blob/1bbf9f9c4dbfea3eea2dc151192ee1f31c6de686/.github/workflows/lint.yaml#L11-L25 . It is based on the fantastic curlylint project.

justinmayer added this to the 3.8.0 milestone Oct 24, 2018

justinmayer added the awaiting review label Nov 1, 2018

avaris approved these changes Nov 1, 2018

View reviewed changes

strburst force-pushed the theme_format branch from 269f4db to c3286d6 Compare November 5, 2018 01:27

justinmayer removed the awaiting review label Nov 11, 2018

justinmayer modified the milestones: 4.0, Next release Nov 13, 2018

strburst force-pushed the theme_format branch 2 times, most recently from bdfb910 to f6f5087 Compare November 14, 2018 04:20

strburst force-pushed the theme_format branch from f6f5087 to 59750b3 Compare November 14, 2018 05:10

mosra reviewed Nov 16, 2018

View reviewed changes

justinmayer modified the milestones: Next patch release, Next minor release Nov 16, 2018

justinmayer modified the milestones: Next minor release, Near-term Jul 5, 2019

justinmayer added the in progress label Apr 16, 2020

justinmayer modified the milestones: Pelican 4.5, Pelican Future Jul 29, 2020

justinmayer added the needs conflict resolution label Jul 26, 2023

justinmayer removed this from the Pelican 4.9 milestone Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize formatting of sample theme Jinja templates #2379

Normalize formatting of sample theme Jinja templates #2379

strburst commented Jul 8, 2018 •

edited

strburst commented Jul 14, 2018

avaris left a comment

justinmayer commented Nov 12, 2018

strburst commented Nov 14, 2018

mosra Nov 16, 2018

mosra Nov 16, 2018

mosra commented Nov 16, 2018

strburst commented Nov 20, 2018

mosra commented Nov 26, 2018

kdeldycke commented Sep 3, 2020

	<li><a href="/tag/oh.html">Oh Oh Oh</a></li>
	<li><a href="/tag/oh.html">Oh Oh Oh</a></li>


		</footer><!-- /.post-info --> <p>You're mutually oblivious.</p>
		</footer><!-- /.post-info --> <p>You're mutually oblivious.</p>

	</footer><!-- /.post-info --> <p>You're mutually oblivious.</p>
	</footer><!-- /.post-info -->
	<p>You're mutually oblivious.</p>

Normalize formatting of sample theme Jinja templates #2379

Are you sure you want to change the base?

Normalize formatting of sample theme Jinja templates #2379

Conversation

strburst commented Jul 8, 2018 • edited

Description

Testing

Implementation note

strburst commented Jul 14, 2018

avaris left a comment

Choose a reason for hiding this comment

justinmayer commented Nov 12, 2018

strburst commented Nov 14, 2018

mosra Nov 16, 2018

Choose a reason for hiding this comment

mosra Nov 16, 2018

Choose a reason for hiding this comment

mosra commented Nov 16, 2018

strburst commented Nov 20, 2018

mosra commented Nov 26, 2018

kdeldycke commented Sep 3, 2020

strburst commented Jul 8, 2018 •

edited