Use template to generate TOC #467

jacwah · 2017-10-08T19:51:23Z

Instead of directly writing html from Rust, use a Handlebars template to generate the table of contents. This requires build a separate json object from chapters. To remove the duplicated data, the second commit adapts navigation to use the new toc json object instead, and is an alternative to #465.

Pros:

toc is easy to edit and customisable
code is more maintainable

Cons:

the template generates funky html with lots of whitespace (will look for a solution)
complicated template -- some handlebars knowledge required
book chapters are processed in two steps (can probably join them to cut down code)

I'm not quite sure how to organise the code. Perhaps toc_json.rs could be merged with helpers/toc.rs. I'm also not sure when to trust the code and use unwrap and when passing along errors is preferred.

#458

Michael-F-Bryan

Overall I really like what you're doing in this PR! It definitely should help to make the rendering side of things easier to reason about in the long run 👍

I've raised a couple points where we can improve readability. It's mainly small things like giving a function a more descriptive name or pulling things out into temporary variables so the next guy knows how things what the reasoning is behind a chunk of code.

Also, would adding in a test or two be in the scope of this PR? If we can check that the output generated using a template for the TOC is similar to the current format then that'll help give us confidence that the code works as intended. It's also really useful for detecting accidental regressions in the future.

Michael-F-Bryan · 2017-11-14T14:02:25Z

src/renderer/html_handlebars/hbs_renderer.rs

+        let mut chapter = BTreeMap::new();
+
+        match *item {
+            BookItem::Affix(ref ch) => {


Do you think it's worth pulling the contents for each of these match arms out into their own functions? I know it's more the way rustfmt has formatted your code than anything else, but that rightward drift is getting a tad extreme.

Also, is it just me or are the branches for BookItem::Affix and BookItem::Chapter almost identical?

Michael-F-Bryan · 2017-11-14T14:07:00Z

src/renderer/html_handlebars/helpers/navigation.rs

+    -> Result<Option<&'a serde_json::Map<String, serde_json::Value>>, RenderError> {
+    let chapter =
+        chapter.as_object()
+               .ok_or_else(|| RenderError::new("Chapter is not an object"))?;


You could probably collapse this all into a single statement. Something along the lines of:

if let Some(link) = chapter.as_object().and_then(|ch| ch.get("link")) { ... }

That would also help simplify the return signature considerably because you're only returning an Option. Returning an error and an option in this case probably won't give give us anything extra because the error branch will be ignored (we couldn't find the thing we were looking for).

Do you mean find_chapter should silently ignore if chapter is not an object or if chapter.children is not an array?

My thinking was that you're trying to find a chapter called needle in some JSON value. If the value isn't an object then as far as the function is concerned it can't find our needle.

What would we be able to do if chapter wasn't an object (or chapter.children isn't an array) besides halting with an error?

I think we should halt if find_chapter discovers a type error at runtime. Just returning None might make an issue with node creating harder to discover and debug.

I like that reasoning 👍

I guess you raise the point that it'd be better if we could work with something a bit more strongly typed than a miscellaneous JSON value so those sorts of type errors are impossible. That's something for a later PR though.

Michael-F-Bryan · 2017-11-14T14:13:24Z

src/renderer/html_handlebars/helpers/toc.rs

+
+/// Link to the current page.
+fn active_link(rc: &RenderContext) -> Result<String, RenderError> {
+    Ok(Path::new(rc.evaluate_absolute("path")?


I know this is picky, but would you be able to break this out into multiple lines with a couple well-named temporary variables?

Michael-F-Bryan · 2017-11-14T14:16:14Z

src/renderer/html_handlebars/helpers/toc.rs

-                // write to the handlebars template
-                rc.writer.write_all(markdown_parsed_name.as_bytes())?;
-            }
+        if active_link(rc)? == link {


Any chance you can pull this active_link(rc)? out into a temporary variable? Conditions both having side effects and the possibility of failing feels kinda odd.

Michael-F-Bryan · 2017-11-14T14:21:18Z

src/renderer/html_handlebars/helpers/toc.rs

-                // render markdown to html
-                let mut markdown_parsed_name = String::with_capacity(name.len() * 3 / 2);
-                html::push_html(&mut markdown_parsed_name, parser);
+    let has_link = if let Some(link) = link.as_str() {


Is there a simpler way to represent this? I feel like a let x = if let Some(y) = z { /* massive block */ true } else { false } would make the control flow a little hard to follow.

What about something like this (rough pseudo-code):

let link = rc.evaluate("link")?.clone(); if link.is_some() { // open anchor tag if active_link(rc)? == link { // add the "active" class } } // write contents if link.is_some() { // close anchor tag }

Michael-F-Bryan · 2017-11-14T14:31:08Z

src/renderer/html_handlebars/toc_json.rs

+        map.insert("section".to_owned(), json!(section));
+    }
+
+    if let Some(name) = item.get("name") {


It may be a good idea to pull the body of this if let block out into its own little helper function so you can give it a descriptive name. That way the next guy to work on this section of the codebase can figure out what's happening and the reasoning behind it at a glance.

To be honest, I'm not 100% on what it does, this is all copied from the old code. Seems like it strips formatting from the name for some reason.

Michael-F-Bryan · 2017-11-14T14:36:21Z

src/renderer/html_handlebars/toc_json.rs

+        .replace("\\", "/")
+}
+
+/// Set name etc for a chapter.


This should probably get a more detailed doc-comment. By reading through the code it looks like we're extracting specific things from item and inserting them into map, but a fairly generic name like set_props() doesn't really do much to tell you that.

Michael-F-Bryan · 2017-11-14T14:37:23Z

src/renderer/html_handlebars/toc_json.rs

+    }
+}
+
+pub fn from_chapters(chapters: &[BTreeMap<String, String>]) -> Result<serde_json::Value> {


What exactly are we constructing "from chapters" here?

I was thinking toc::from_chapters, but that can be amended.

Ah that's smart. It would make perfect sense from a caller's perspective because it's convention to call functions qualified by their module. The name makes sense, but do you reckon it's worth adding in a short doc-comment to briefly summarize what it's doing?

Michael-F-Bryan · 2017-11-14T14:42:10Z

src/renderer/html_handlebars/toc_json.rs

+}
+
+/// Extend or collapse levels to reach a certain depth.
+fn set_level(level: usize, levels: &mut Vec<serde_json::Value>) {


Hmm... Is this function just turning a list of JSON objects into a tree-like structure, stubbing it out with empty objects if the list of levels passed in isn't big enough? If that's the case then we may want to give it a more descriptive name.

Would it be better to rewrite this in a more functional style? So you'd take in a &[Value] then return a single Value which points to the tree's root.

I hope the updated code is more clear in what it does. I tried finding better names and adding comments where relevant.

Michael-F-Bryan · 2017-11-14T14:44:04Z

src/renderer/html_handlebars/toc_json.rs

+            current.insert("spacer".to_owned(), json!(true));
+        } else {
+            let level = if let Some(s) = item.get("section") {
+                ::std::cmp::max(s.matches('.').count(), 1)


Do you reckon it's worth importing std::cmp up the top so we don't need to specify the full function path?

Michael-F-Bryan

Overall I really like what you're doing in this PR! It definitely should help to make the rendering side of things easier to reason about in the long run 👍

I've raised a couple points where we can improve readability. It's mainly small things like giving a function a more descriptive name or pulling things out into temporary variables so the next guy knows how things what the reasoning is behind a chunk of code.

Also, would adding in a test or two be in the scope of this PR? If we can check that the output generated using a template for the TOC is similar to the current format then that'll help give us confidence that the code works as intended. It's also really useful for detecting accidental regressions in the future.

Michael-F-Bryan · 2017-11-14T15:00:42Z

Sorry for taking a while to get back to you on this @jacwah! I've added a couple small points on adjusting control flow or naming (names are hard!) to hopefully make things a bit more readable. The HTML renderer has typically been pretty messy, so if we can clean it up a bit while we're working in the same earlier then that's a bonus.

We may also want to throw in a test or two to double check the TOC actually does what we intend it to do.

As far as code organisation goes you'll probably want to keep all TOC-related code in the same file. What is the difference meant to be between helpers/toc.rs and toc_json.rs?

jacwah · 2017-11-14T15:07:44Z

@Michael-F-Bryan I appreciate the feedback! I'll get back to you when I've fixed the issues you pointed out.

jacwah · 2017-11-14T23:54:57Z

@Michael-F-Bryan I've added some more commits trying to adress your concerns. There are now some tests where I thought they would make sense. The coverage is not complete, but shows the main functionality. If you have any more ideas I'm all ears!

What do you think about the generated html? I was thinking about stripping whitespace from the toc template when it's loaded, or perhaps adding a pretty printing or minimizing build step. Maybe that's another PR though.

Michael-F-Bryan · 2017-11-15T00:23:02Z

src/renderer/html_handlebars/toc_json.rs

+
+/// Turn a flat chapters array into a tree json structure. Each node represents
+/// a section and its subsections.
+pub fn make_toc_tree(chapters: &[BTreeMap<String, String>]) -> Result<serde_json::Value> {


I like the more descriptive name and the doc-comment you've added. Now we can tell at a glance what this function does :)

Michael-F-Bryan · 2017-11-15T00:30:50Z

Thanks for adding in the extra tests @jacwah. Don't worry too much about not having 100% coverage, it's more a case of incrementally cleaning up the code and decreasing technical debt.

I don't think the generated output being non-optimal is an issue at this point, you don't generally look at the raw HTML anyway. Either way we can push that off until a later PR.

Michael-F-Bryan

It all looks pretty good to me. There's one point where we could probably use error-chain instead of returning an io::Error, but other than that this is pretty much ready to merge.

Michael-F-Bryan · 2017-11-15T08:03:22Z

src/book/mod.rs

@@ -262,6 +262,10 @@ impl MDBook {
        let mut index = File::create(&themedir.join("index.hbs"))?;
        index.write_all(theme::INDEX)?;

+        // toc.hbs
+        let mut toc = File::create(&themedir.join("toc.hbs"))?;


From memory File::create() takes anything which is AsRef<Path>, so you should be able to drop the &'s from File::create(&themedir.join("toc.hbs")) and friends.

Michael-F-Bryan · 2017-11-15T08:05:56Z

src/renderer/html_handlebars/hbs_renderer.rs

+    chapter.insert("name".to_owned(), item.name.to_owned());
+
+    let path = item.path.to_str().ok_or_else(|| {
+        io::Error::new(io::ErrorKind::Other, "Could not convert path to str")


I believe we're using error-chain here, so you should be able to do chain_err(|| "Could not convert path to str") instead of using an io::Error.

Michael-F-Bryan · 2017-11-15T15:08:42Z

I feel like we're about ready to merge this PR. @jacwah, it looks like master has moved since you started this PR so you'll need to do a rebase (git rebase master && git push --force) and fix any conflicts along the way.

An issue that was raised is it may be a good idea to use something more strongly typed than a serde_json::Value. That way a lot of the error checking becomes redundant and can be replaced by a single deserialization from a Value at the start of the handlebars handler, then a conversion back to a Value at the end. We'll leave that for a later PR though.

Michael-F-Bryan · 2017-11-18T15:33:01Z

@jacwah, you mentioned that bits of this are an alternative to #465, so merging #465 means we've now got a conflict because this PR and that one propose different refactorings to src/renderer/html_handlebars/helpers/navigation.rs.

What do you think the easiest way to resolve this would be?

Dylan-DPC-zz · 2019-04-23T18:29:20Z

@jacwah any updates on this?

jacwah · 2019-05-07T15:02:42Z

@Dylan-DPC I did all this a long time ago. At the moment, I don't really have the time to refamiliarize myself with the code and resolve accumulated merge conflicts. If someone else finds my work here useful, they should feel free to finish it. Else, I am ok with closing this PR.

Dylan-DPC-zz · 2019-05-07T22:35:47Z

@jacwah this PR was created a while back and it isn't clear what we should do next this, so closing it would be a good idea at this point. Thanks for contributing. If you want to take another go at it sometime in future, feel free to submit a new PR :)

Michael-F-Bryan self-requested a review November 11, 2017 08:38

Michael-F-Bryan suggested changes Nov 14, 2017

View reviewed changes

Michael-F-Bryan reviewed Nov 15, 2017

View reviewed changes

jacwah force-pushed the toc branch from 73a940f to 8aa5388 Compare November 15, 2017 17:28

jacwah added 12 commits November 15, 2017 18:35

Use template for TOC

151d8be

navigation: use toc instead of chapters

e448c80

Rustfmt toc_json and navigation

967a95a

Refactor make_chapters

07faefd

Refactor active_link

c6414aa

Refactor toc::link

5dbd4a5

Refactor creating toc json structure

6af206a

Add toc json test

38091a4

Refactor toc node creation

f96b5f5

Add toc html test

07e6eb8

Remove unnecessary &s

24c988a

Change io::Error::new to chain_err()

6071f7b

jacwah force-pushed the toc branch from 8aa5388 to 6071f7b Compare November 15, 2017 17:41

Michael-F-Bryan mentioned this pull request Nov 18, 2017

Spaghetti Time! #458

Open

6 tasks

Michael-F-Bryan approved these changes Nov 18, 2017

View reviewed changes

suhr mentioned this pull request Apr 4, 2018

WIP: Add support for virtual chapters #592

Closed

8 tasks

Dylan-DPC-zz closed this May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use template to generate TOC #467

Use template to generate TOC #467

jacwah commented Oct 8, 2017 •

edited

Loading

Michael-F-Bryan left a comment

Michael-F-Bryan Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

jacwah Nov 14, 2017

Michael-F-Bryan Nov 15, 2017

jacwah Nov 15, 2017

Michael-F-Bryan Nov 15, 2017

Michael-F-Bryan Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

jacwah Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

jacwah Nov 14, 2017

Michael-F-Bryan Nov 15, 2017

Michael-F-Bryan Nov 14, 2017

jacwah Nov 14, 2017

Michael-F-Bryan Nov 14, 2017

Michael-F-Bryan left a comment

Michael-F-Bryan commented Nov 14, 2017 •

edited

Loading

jacwah commented Nov 14, 2017

jacwah commented Nov 14, 2017

Michael-F-Bryan Nov 15, 2017

Michael-F-Bryan commented Nov 15, 2017

Michael-F-Bryan left a comment

Michael-F-Bryan Nov 15, 2017

Michael-F-Bryan Nov 15, 2017

Michael-F-Bryan commented Nov 15, 2017

Michael-F-Bryan commented Nov 18, 2017 •

edited

Loading

Dylan-DPC-zz commented Apr 23, 2019

jacwah commented May 7, 2019

Dylan-DPC-zz commented May 7, 2019

Use template to generate TOC #467

Use template to generate TOC #467

Conversation

jacwah commented Oct 8, 2017 • edited Loading

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Nov 14, 2017 • edited Loading

jacwah commented Nov 14, 2017

jacwah commented Nov 14, 2017

Choose a reason for hiding this comment

Michael-F-Bryan commented Nov 15, 2017

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Nov 15, 2017

Michael-F-Bryan commented Nov 18, 2017 • edited Loading

Dylan-DPC-zz commented Apr 23, 2019

jacwah commented May 7, 2019

Dylan-DPC-zz commented May 7, 2019

jacwah commented Oct 8, 2017 •

edited

Loading

Michael-F-Bryan commented Nov 14, 2017 •

edited

Loading

Michael-F-Bryan commented Nov 18, 2017 •

edited

Loading