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

Use Tera templates for rustdoc. #86157

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Use Tera templates for rustdoc. #86157

merged 1 commit into from
Jun 21, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jun 9, 2021

Replaces a format!() call in layout::render with a template
expansion. Introduces a templates field in SharedContext so parts
of rustdoc can share pre-rendered templates.

This currently builds in a copy of the single template available, like
with static files. However, future work can make this live-loadable with
a perma-unstable flag, to make rustdoc developers' work easier.

Part of #84419.

Demo at https://hoffman-andrews.com/rust/tera/std/string/struct.String.html.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 9, 2021
@rust-log-analyzer

This comment has been minimized.

@@ -1482,6 +1482,17 @@ dependencies = [
"regex",
]

[[package]]
name = "globwalk"
Copy link
Member

Choose a reason for hiding this comment

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

Oh oof I forgot tera uses globwalk :( it's extremely slow: rust-lang/docs.rs@9c9647d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't actually use globwalk in this PR, we are manually loading files the same way the linked docs.rs PR does.

@GuillaumeGomez
Copy link
Member

For now, I don't see much advantages to this change, but at least it's a good start. What about the size of generated files and performance comparison?

@jyn514
Copy link
Member

jyn514 commented Jun 18, 2021

@GuillaumeGomez the advantage is it's much easier to read than repeated write calls

@GuillaumeGomez
Copy link
Member

In this case not really since only the big one has been moved.

@jsha
Copy link
Contributor Author

jsha commented Jun 20, 2021

It's true this is mainly an initial setup to make subsequent templatizations possible, and to give a feel for what the template language will feel like. And the one format call here is the most template-like thing we currently have, since it already has named fields for interpolation. But even in this initial change you can see some of the benefits in clarity and readability. Consider:

Old:

        logo = {
            if layout.logo.is_empty() {
                format!(
                    "<a href='{root}{path}index.html'>\
                     <div class='logo-container rust-logo'>\
                     <img src='{static_root_path}rust-logo{suffix}.png' alt='logo'></div></a>",
                    root = page.root_path,
                    path = ensure_trailing_slash(&layout.krate),
                    static_root_path = static_root_path,
                    suffix = page.resource_suffix
                )
            } else {
                format!(
                    "<a href='{root}{path}index.html'>\
                     <div class='logo-container'><img src='{logo}' alt='logo'></div></a>",
                    root = page.root_path,
                    path = ensure_trailing_slash(&layout.krate),
                    logo = layout.logo
                )
            }
        },

New:

<a href='{{page.root_path | safe}}{{krate_with_trailing_slash | safe}}index.html'>
    <div class='logo-container'>
    <img src='
        {%- if layout.logo -%}
        {{layout.logo}}
        {%- else -%}
        {{static_root_path | safe}}rust-logo{{page.resource_suffix}}.png
        {%- endif -%}
        ' alt='logo'>
    </div>
</a>

That's 20 lines vs 11 lines, and the 11 line version also removes some redundancy. Also, the HTML in the template version is easier to read without backslashes in front of all the quotes.

Performance: The difference so far will be quite negligible, because the amount of code moved in this PR is small. As we discussed in #84419, we may need to take a bit of a leap of faith performance-wise since the overall performance of templatizing everything won't be clear until we're much further along.

Byte size: struct.String.html is 54,713 bytes gzipped, vs 54,594 on master. Note that this branch doesn't have special treatment for removing excess whitespace. I used {%- and -%} where appropriate to take advantage of newline control, but the natural layout includes newlines and indents even between constant HTML elements, where there's no natural place for {%- and -%}. I think as a followup we should plan to pass the templatized content through a very basic parser that can remove HTML whitespace. If you'd like that as a prereq to landing this branch, let me know.

@GuillaumeGomez
Copy link
Member

Byte size: struct.String.html is 54,713 bytes gzipped, vs 54,594 on master. Note that this branch doesn't have special treatment for removing excess whitespace. I used {%- and -%} where appropriate to take advantage of newline control, but the natural layout includes newlines and indents even between constant HTML elements, where there's no natural place for {%- and -%}. I think as a followup we should plan to pass the templatized content through a very basic parser that can remove HTML whitespace. If you'd like that as a prereq to landing this branch, let me know.

Yes please. I'm honestly very afraid of the whole "remove whitespaces from HTML". In this case, we even need to re-parse it. Even though I really like the whole templating idea, the downside is really worrying me. But I may be completely wrong. So I'd prefer to have the whitespaces removed from HTML before merging.

@jyn514
Copy link
Member

jyn514 commented Jun 20, 2021

@GuillaumeGomez are you worried about reparsing? or about the change in file size? If you're just worried about reparsing, I don't see why this has to be blocked on removing the whitespace.

@jsha
Copy link
Contributor Author

jsha commented Jun 21, 2021

Alright, with the latest updated, struct.String.html is 54,597 gzipped bytes, 2 bytes smaller than on master. :-)

No parsing required! The trick was to add {#- -#} to the end of each line. {# is a comment, and adding - triggers the whitespace removal path.

This is still not completely optimal, since it incentivizes putting a whole HTML tag on a single line, even when you might prefer to split out attributes on their own line. If you split out attributes on their own line, you're forced to choose between putting {#- -#} at the end of each line, which will concatenate the attributes and horribly break everything, or not putting any whitespace control in at all and incurring some extra bytes. Still, I think it's an okay tradeoff.

One thing that might be cool longer term is to see about getting Django's spaceless feature implemented in Tera (though I think we'd also want a special mode that's aware of HTML templates and collapses N whitespaces into 1 space character).

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

One thing that might be cool longer term is to see about getting Django's spaceless feature implemented in Tera (though I think we'd also want a special mode that's aware of HTML templates and collapses N whitespaces into 1 space character).

That would be amazing! Can you open an issue on tera?

Replaces a format!() call in layout::render with a template
expansion. Introduces a `templates` field in SharedContext so parts
of rustdoc can share pre-rendered templates.

This currently builds in a copy of the single template available, like
with static files. However, future work can make this live-loadable with
a perma-unstable flag, to make rustdoc developers' work easier.
@jsha
Copy link
Contributor Author

jsha commented Jun 21, 2021

Done: Keats/tera#641

I also figured out how to do single whitespace for attributes: {# -#} instead of {#- -#} (that is, remove whitespace to the right, but not to the left). Switched to that, and added a STYLE.md discussing style for templates.

<meta name="keywords" content="{{page.keywords}}"> {#- -#}
<title>{{page.title}}</title> {#- -#}
<link rel="stylesheet" type="text/css" {# -#}
href="{{static_root_path | safe}}normalize{{page.resource_suffix}}.css"> {#- -#}
Copy link
Member

Choose a reason for hiding this comment

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

This is weird to have two different syntaxes depending if we're in a tag or not. Oh well...

@GuillaumeGomez
Copy link
Member

Well, the final result is really not great if we have to add {# -#}/{#- -#} at every line... However if the end size is equal/smaller, then I won't block on this.

Let's just hope the "whitespace cleaner" feature will be implemented shortly in tera. :)

In any case, thanks a lot for working on this, it'll allow to improve the code a lot!

@bors: r=jyn514,GuillaumeGomez

@bors
Copy link
Contributor

bors commented Jun 21, 2021

📌 Commit cd0f931 has been approved by jyn514,GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2021
@bors
Copy link
Contributor

bors commented Jun 21, 2021

⌛ Testing commit cd0f931 with merge 9d93819...

@bors
Copy link
Contributor

bors commented Jun 21, 2021

☀️ Test successful - checks-actions
Approved by: jyn514,GuillaumeGomez
Pushing 9d93819 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2021
@bors bors merged commit 9d93819 into rust-lang:master Jun 21, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 21, 2021
@jsha jsha deleted the tera branch June 22, 2021 00:14
GuillaumeGomez pushed a commit to ijackson/rust that referenced this pull request Jul 21, 2021
In rust-lang#86157

    cd0f931
    Use Tera templates for rustdoc.

dropped the following transformation from the keys of the default
settings element's `data-` attribute names:

    .map(|(k, v)| format!(r#" data-{}="{}""#, k.replace('-', "_"), Escape(v)))

The `Escape` part is indeed no longer needed, because Tera does that
for us.  But the massaging of `-` to `_` is needed, for the (bizarre)
reasons explained in the new comments.

I have tested that the default theme function works again for me.  I
have also verified that passing

    --default-theme="zork&"

escapes the value in the HTML.

Closes rust-lang#87263.

CC: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 22, 2021
…Gomez

rustdoc: Restore --default-theme, etc, by restoring varname escaping

In rust-lang#86157

    cd0f931
    Use Tera templates for rustdoc.

dropped the following transformation from the keys of the default settings element's `data-` attribute names:

    .map(|(k, v)| format!(r#" data-{}="{}""#, k.replace('-', "_"), Escape(v)))

The `Escape` part is indeed no longer needed, because Tera does that for us.  But the massaging of `-` to `_` is needed, for the (bizarre) reasons explained in the new comments.

I have tested that the default theme function works again for me.  I have also verified that passing (in shell syntax)

    '--default-theme="zork&"'

escapes the value in the HTML.

Closes rust-lang#87263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants