rustdoc: Migrate sidebar rendering to Askama by clubby789 · Pull Request #108784 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation45 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
cc #108757
Renders the sidebar for documentation using an Askama template
r? @jsha
(rustbot has picked a reviewer for you, use r? to override)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the rustdoc team, which will review and decide on the PR/issue.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for working on this! This is gonna be awesome. Some early feedback: This moves too much of the logic into the template itself. I would rather see most of the logic continue to be on the Rust side, filling out fields of a Template struct that can then be straightforwardly interpolated into the template using very little flow control and very few method calls. Essentially, the Template struct should be an abstraction layer between "what we want to express" and "what gets rendered as HTML." I think this will have the happy side benefit of making the code easier to understand than it is today, since the Template struct can express intention more clearly without the HTML getting in the way.
As some additional justification for why it's valuable to keep the logic on the Rust side:
- Tooling like jump-to-definition, hover to see types, rustfmt, etc, is not available when viewing an HTML template
- Error messages for mistakes in Rust-inside-HTML-template are harder to understand
- Indentation is hard to manage when going back and forth between HTML code and Rust code within the same HTML template.
I gave an example below of how I think we can add fields to the Sidebar struct such that the HTML template becomes much clearer. I can go through and provide some additional thoughts as I have time, if that would be helpful.
In the STYLE.md doc I tried to express some of these preferences but it's a little easier to see them concretely in action.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see this simplified as:
{%- if !title.is_empty() %-}
<h2 class="location">
<a href="#">{{ title_prefix }}{{ title}}</a>
</h2>
{%- endif -%}
This would necessitate giving Sidebar a title: &str and title_prefix: &str field, and moving the it.is_foo() logic, plus the "Crate " / "Module " / "" logic back into the Rust code.
Comment on lines 17 to 25
| {%- if it.is_crate() -%} |
|---|
| {%- match cx.cache().crate_version -%} |
| {% when Some with (version) %} |
| {%- else -%} |
| {% endmatch -%} |
| {%- endif %} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As another example, this could be simplified with the addition of two fields to Sidebar:
is_crate_page: bool,
crate_version: &'str, // only present on crate pages
Then the template would become:
{%- if is_crate_page -%}
<ul class="block"> {#- -#}
<li class="version">Version {{version}}</li> {#- -#}
<li><a id="all-types" href="all.html">All Items</a></li> {#- -#}
</ul> {#- -#}
{%- endif -%}
By the way, note that I use the double-sided whitespace-eliminating comment {#- -#} instead of the single-sided on {# -#}. That allows the comment to stand off from the HTML by a space, which I think is a little more readable.
The single-sided comment {# -#} is needed when an HTML tag with attributes is spread over multiple lines, in order to preserve one space between attributes. Otherwise the attributes would get jammed together.
This comment has been minimized.
Hopefully closer to what you're looking for now. I've made a fair amount of use of macros (converted from regular functions in the original code), but it might be simpler/more readable to just inline the loops here.
@rustbot ready
Given tests pass and the majority of the sidebar has been moved over, taking this off WIP
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Thanks for the revisions.
As you pointed out, it would be nicer to genericize the link blocks and get rid of the macros. I'm thinking something like:
struct<'a> LinkBlock<'a> {
heading: &'a str,
heading_href: &'a str,
links: Vec<Link<'a>>, // Or Iterator?
}
struct<'a> Link<'a> {
href: &'a str,
name: &'a str,
}
And then Sidebar would have a Vec<LinkBlock>.
I believe this would allow us to collapse the multiple different .html files for the different flavors of sidebar, in favor of a single sidebar.html that iterates across multiple LinkBlocks.
I managed to move it all over to a single sidebar.html with LinkBlock and Link structs. Having only one layer of templates means I could remove all the |safe uses in the sidebar, which I was a bit worried about.
I also moved just about all of the sidebar-relevant code into a new module, as I was finding mod.rs a bit difficult to navigate with the current size.
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, I love the latest refactoring. It adds a lot of clarity to both the template and the code.
Looks like you've got a couple of test cases failing:
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/crate-version-escape" "/checkout/tests/rustdoc/crate-version-escape.rs"
stdout: none
--- stderr -------------------------------
5: @has check failed
`XPATH PATTERN` did not match
// @has 'foo/index.html' '//li[@class="version"]' 'Version <script>alert("hi")</script>'
Encountered 1 errors
------------------------------------------
---- [rustdoc] tests/rustdoc/crate-version.rs stdout ----
error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/crate-version" "/checkout/tests/rustdoc/crate-version.rs"
stdout: none
--- stderr -------------------------------
3: @has check failed
`XPATH PATTERN` did not match
// @has 'crate_version/index.html' '//*[@class="version"]' 'Version 1.3.37'
Encountered 1 errors
------------------------------------------
failures:
[rustdoc] tests/rustdoc/crate-version-escape.rs
[rustdoc] tests/rustdoc/crate-version.rs
Also one thing I like to do when performing a refactoring like this is generate before-and-after output for ./x.py doc library/std and diff them; if there are no diffs, the refactoring had no unintended side effects, at least over the std docs.
Looks like you've got a couple of test cases failing:
Interesting, they pass locally. I can see the problem by eye, but odd that it wasn't picked up on my tests.
Also one thing I like to do when performing a refactoring like this is generate before-and-after output for ./x.py doc library/std and diff them
I've been attempting to do this, but having difficulty since the outputted HTML is all on one line and always seems to have slightly different URLs for resources. Are there any good tools for diffing in this situation?
Also looks like there have been some changes to doc layout since the current bootstrap compiler which are getting in the way of diffing 🙁
Ah, upstream changed whitespace = "suppress" so the space after Version was squashed. Rebased and fixed
one thing I like to do when performing a refactoring like this is generate before-and-after output for ./x.py doc library/std and diff them
I've been attempting to do this, but having difficulty since the outputted HTML is all on one line and always seems to have slightly different URLs for resources. Are there any good tools for diffing in this situation?
Yeah, the all-one-line thing makes it hard. Usually I'll use plain diff to find files that differ, and then use vimdiff on a specific file to find differences, since it's good at showing intra-line diffs.
Sometimes I'll do sed -i s/>/\n/g to get things roughly on separate lines and then diff the result (of course this is not a valid HTML transformation but it's enough to see patterns). I've also tried using tidy on each side of the diff, but tidy is pretty aggressive in its transforms and sometimes changes so much that I can't tell what's going on.
When you say "always seems to have slightly different URLs for resources" - are you talking about the static files in rustdoc.static/? Those should be steady so long as the file contents aren't changing. If you make sure your branch is up to date with master, and clear out the generated files between runs, those URLs shouldn't change.
The latest test failures seem related to escaping of link fragments in href:
core/primitive.reference.html:1: broken link fragment `#impl-CoerceUnsized%3C%26'a+U%3E-for-%26'b+T` pointing to `core/primitive.reference.html`
Taking just the fragment, we have (old/new):
#impl-CoerceUnsized%3C%26'a+U%3E-for-%26'b+T
#impl-CoerceUnsized%3C%26'a+U%3E-for-%26'b+T
It looks like the ' is getting encoded using an HTML entity ('). That's not really necessary for us because we use double quotes (") to surround our attributes, so single quotes (') are not relevant. But Askama doesn't know that and is just trying to escape all possibly-relevant characters. I think using |safe for the LinkBlocks' href should fix it.
Note that URL escaping was recently touched in #107284 and #107655.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
📌 Commit 85ae75aba179e44de205556d7a0acee1e0bd1ef0 has been approved by jsha
It is now in the queue for this repository.
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
@bors r-
Just needs to remove unneeded -.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Fixed, and also changed the indentation to be more in line with the style guide
It looks better indeed. You also removed some unneeded {# #}, well done. Thanks!
@bors r=jsha,GuillaumeGomez rollup
📌 Commit 2f166d1 has been approved by jsha,GuillaumeGomez
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…uillaumeGomez
rustdoc: Migrate sidebar rendering to Askama
Renders the sidebar for documentation using an Askama template
bors added a commit to rust-lang-ci/rust that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the rustdoc team, which will review and decide on the PR/issue.