Refactor hbs renderer #335

Michael-F-Bryan · 2017-06-15T10:15:25Z

In my "plugins" branch I'm wanting to experiment with different ways of doing plugins (for pre-processing, post-processing, different backends, etc) and in the process I needed to refactor the HTMLHandlebars renderer a little bit. If you reckon they'll be useful, feel free to merge the changes into master :)

All I did was pull bits and pieces of the HTMLHandlebars::render() method out into their own helper functions so it's easier to wrap your head around.

steveklabnik · 2017-06-15T14:24:01Z

All I did was pull bits and pieces of the HTMLHandlebars::render() method out into their own helper functions so it's easier to wrap your head around.

Sounds great to me! Tests are failing though.

Michael-F-Bryan · 2017-06-15T19:53:10Z

Hmm, that's awfully strange. I looked at the Travis build and apparently that "X" is for commit cdeec22, yet the failing commit for this PR is b7aa78c... Additionally, when you follow the link to Travis, the PR is my "[WIP] Plugin Experiment" (#336). Maybe Travis hit a bug because there are multiple PRs from the same person, both sharing a couple commits?

I should be able to fix this up tomorrow so it's ready to merge.

Michael-F-Bryan · 2017-06-15T23:03:01Z

@steveklabnik, travis should be happy now. I think it was just getting confused because my PRs had a similar history and were made around the same time. The first 3 or 4 builds have all passed without any issues, so when they're all finished this should be good to merge.

budziq · 2017-06-17T09:15:47Z

src/renderer/html_handlebars/hbs_renderer.rs

@@ -182,6 +182,16 @@ impl HtmlHandlebars {
        handlebars.register_helper("previous", Box::new(helpers::navigation::previous));
        handlebars.register_helper("next", Box::new(helpers::navigation::next));
    }
+
+    fn copy_additional_css(&self, book: &MDBook) -> Result<(), Box<Error>> {


Hi just minor suggestion. This function copies also additional js files so maybe rename it to something like copy_custom_files`` or *_css_js`

Cheers for picking up on that! I had another look at the names I'm using to make sure they reflect what the function is actually doing. In the end I went with the slightly longer copy_additional_css_and_js.

budziq · 2017-06-17T09:20:30Z

Wow that's a lot of awesome work!

Michael-F-Bryan · 2017-06-18T01:54:03Z

I'm not quite happy with the render_item() method (just look at that function signature!), but I think other than that I've left the hbs_renderer.rs module tidier than I found it. @budziq or @steveklabnik, any chance you can give this one final review?

budziq · 2017-06-18T07:01:33Z

Hi. I'm on mobile only for upcoming few days so only a birds eye view on my part (and nothing speccific I'm afraid as viewing diffs on mobile github is quite a chore).

idealy we would loose the out params
have a minimal RenderContext param that might contain all required info and enclose mutable state if there is no other choice.
return something consumable in the Result instead of (). Maybe a some RenderItem type if we can avoid mutable state.
most likely the index handling part could be extracted to another function (possibly dropping the index out param)
Possibly it might be nice to split the rendering and write/build phases of the Renderer trait. One would accumulate the state that might me unavoidable for other output types like pdf. And second could write/build.

These are just some random thoughts without much insight so fell free to disregard any of those.

azerupi · 2017-06-18T08:55:48Z

I think this PR is going to have some conflicts with #328
Maybe we could try to combine both?

budziq · 2017-06-18T10:04:31Z

src/renderer/html_handlebars/hbs_renderer.rs

+
+                        let mut content = String::new();
+
+                        let _source = File::open(destination.join(&ch.path.with_extension("html")))


the standalone "?" below looks wierd and the temp var _source could be removed completely.

Oops! That's probably a bug in rustfmt-nightly and I didn't take the time to look over the code for formatting issues before committing.

I think the _source was left over from the original HtmlHandlebars implementation and I didn't notice it wasn't being used because the leading underscore silences unused variable warnings.

budziq · 2017-06-18T10:08:43Z

src/renderer/html_handlebars/hbs_renderer.rs

-                            } else {
-                                None
-                            })
+                    if c.is_ascii() {


this whole block could use a refactor while you are at it.

Yeah, definitely. I was mainly trying to break things up into smaller chunks so it's easier to understand what HtmlHandlebars::render() does, but I may as well clean this up too.

I just had a shot at fixing this up. The issue I'm having is that there aren't any tests to go off of and it's difficult to understand what the underlying algorithm is.

I think it's trying to:

generate an id for each header tag (h1, h2, ...) from its content by

removing any inner tags

ascii lowercasing

replacing spaces with dashes

appending an auto-incremented counter to make sure the id is unique

then wrap the entire thing in an anchor tag so regex can replace the old header with the new one

Can you or @azerupi let me know if I'm on the right track?

budziq · 2017-06-18T10:18:43Z

src/renderer/html_handlebars/hbs_renderer.rs

+        let mut data = make_data(book)?;
+
+        // Print version
+        let mut print_content = String::new();


nit sure if "print" should not be a separate renderer in its own right.

I believe this is for when you'll try to print the html page from your browser, so it's actually a subsection of the HTML renderer. That was my interpretation anyway, either way at the minimum I should pull all print-specific stuff out into its own method.

steveklabnik · 2017-06-19T20:57:02Z

I wanted to say that I largely agree with @budziq

Michael-F-Bryan · 2017-06-20T00:40:06Z

Sorry it's taken me a while to get back to you guys! In #336 I was discussing with @azerupi that I'd like to do a lot of refactoring/restructuring of mdbook over the holidays (my last exam is tomorrow) to make it easier to work on and testable.

As such, the entire Renderer system will probably be getting an overhaul because mdbook is really tightly coupled to the existing HtmlHandlebars renderer. A while back I tried to create a pdf renderer (which eventually spawned latex-rs), but couldn't make much headway because of how the the library is structured and a lot of the implicit assumptions in MDBook and configuration.

have a minimal RenderContext param that might contain all required info and enclose mutable state if there is no other choice.

Good idea, at the very least it'll help make that function signature cleaner, but I don't think it'll deal with the underlying issue that rendering is tightly coupled to the MDBook::write_file() method and a lot of state. Which in turn makes it very hard to test and understand the code. I'm hoping to fix a lot of those state and testability issues in this upcoming refactoring/restructuring.

return something consumable in the Result instead of (). Maybe a some RenderItem type if we can avoid mutable state.

Possibly it might be nice to split the rendering and write/build phases of the Renderer trait. One would accumulate the state that might me unavoidable for other output types like pdf. And second could write/build.

Eventually that's the plan. Rendering should be broken up into a two-step process where you'll convert the in-memory Book into an in-memory representation of the rendered book, doing all the work to turn markdown into html. Then the second step would be to walk your in-memory representation, writing all of it to disk. The way the playpen and other bits of rendering are currently designed it assumes the book has been incrementally written to the file system, so if I were to return an intermediate RenderedItem playpen links would probably be broken because the item hasn't actually been written to disk yet.

There are a couple other things like that in the HtmlHandlebars::render() method and the only real way to fix them would be to rewrite most of the html renderer module to make it more flexible and testable (without needing to create an entire book in a temporary directory). I'll probably end up doing that over the next month or so anyway, but this PR was mainly to make the existing implementation more readable and help future work.

I also accidentally ran `rustfmt` instead of `rustfmt-nightly`, so there are a lot of unnecessary style changes :(

…procedural style

budziq · 2017-06-20T07:31:49Z

@Michael-F-Bryan I haven't had time to really look into your changes yet. Please note that the nits so far were to the legacy implementation that you are trying to refactor so do not feel discouraged. So far the changes seem sound 👍 but taking into consideration the shear volume it will need some time to scan through (hopefully later this week).

But please consider what @azerupi has noted before. Part of these changes conflict with https://github.com/azerupi/mdBook/pull/328. Myybe it would be possible to coordinate with @sunng87

sunng87 · 2017-06-21T14:05:39Z

@budziq @azerupi #328 didn't touch any file in this patch. It should be ok to merge both, I think.

Michael-F-Bryan · 2017-06-22T00:22:06Z

@sunng87, when I get back from work I'll make a local copy of this pull request and try to merge it with what you've done and see if that still compiles.

The files I imported might use something you changed (or vice versa), skimming through what you've done it doesn't look like we'll affect each other but it's probably a good idea to check regardless.

sunng87 · 2017-06-22T02:20:57Z

@Michael-F-Bryan Got it. It's probably caused by some internal changes in handlebars 0.27. Let me take a look later today.

sunng87 · 2017-06-22T15:01:05Z

I can see no conflict and no error in cargo build and cargo test by merging #328 and this. It should be OK to merge both. @Michael-F-Bryan @budziq @azerupi

azerupi · 2017-06-22T15:15:05Z

Thanks @sunng87 for testing!
I will take a look at the two PRs and try to merge them :)

azerupi · 2017-06-23T10:25:10Z

src/renderer/html_handlebars/hbs_renderer.rs

+        Ok(())
+    }
+
+    fn write_custom_file(&self, custom_file: &Path, book: &MDBook) -> Result<(), Box<Error>> {


The name of this function is quite vague and doesn't really tell you what it does. Could you add a short comment above it explaining what it does for other people? Or if you have an idea for a better name..? :)

I'm pretty sure that's just a helper function which will take some path relative to the book's src directory and copy it to the output directory. I've added a doc-comment explaining what it does.

Notably, this takes into account the curly-quotes pull request (rust-lang#305)

Michael-F-Bryan · 2017-06-24T08:21:34Z

@azerupi, I've solved the merge conflict on this (#305 altered a line I also refactored) and added an extra doc comment to explain what that write_custom_file() method does.

Do you think anything else needs to be done on this PR?

azerupi · 2017-06-24T12:44:08Z

No, it's all good for me! :)

…nderer Refactor hbs renderer

Michael-F-Bryan added 4 commits June 15, 2017 17:43

Pulled page rendering out into its own method

0d0deb7

Pulled some more little bits out into their own helper functions

f946ef6

fixed a typo

2568986

Minor refactoring

b7aa78c

Michael-F-Bryan mentioned this pull request Jun 15, 2017

[WIP] Plugin Experiment #336

Closed

Tiny whitespace changes

deab3ba

Michael-F-Bryan force-pushed the refactor-hbs-renderer branch from 7f3a279 to deab3ba Compare June 16, 2017 13:57

budziq reviewed Jun 17, 2017

View reviewed changes

Renamed a couple functions to be more descriptive and ran rustfmt

4af10ce

budziq reviewed Jun 18, 2017

View reviewed changes

Pulled index rendering out into its own method

75f0196

Michael-F-Bryan added 5 commits June 20, 2017 08:54

Introduced a RenderItemContext to make item rendering easier

e2a7ada

I also accidentally ran `rustfmt` instead of `rustfmt-nightly`, so there are a lot of unnecessary style changes :(

Added some tests for the original build_header_links function

ac16d7a

Broke the header link wrapping out into smaller functions

fa95546

Used the Entry API to make id counter incrementing nicer

8c30de1

Cleaned up the filter_map for normalizing id's using a more readable …

33f3bec

…procedural style

azerupi reviewed Jun 23, 2017

View reviewed changes

azerupi approved these changes Jun 23, 2017

View reviewed changes

budziq mentioned this pull request Jun 23, 2017

Make MathJax optional #347

Closed

Michael-F-Bryan added 2 commits June 24, 2017 15:50

Explained what HtmlHandlebars::write_custom_function() does

4c187bc

Merge branch 'upstream/master' into refactor-hbs-renderer

c3dfabd

Notably, this takes into account the curly-quotes pull request (rust-lang#305)

azerupi merged commit b441066 into rust-lang:master Jun 24, 2017

Michael-F-Bryan deleted the refactor-hbs-renderer branch June 24, 2017 13:58

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020

Merge pull request rust-lang#335 from Michael-F-Bryan/refactor-hbs-re…

04cc3b6

…nderer Refactor hbs renderer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor hbs renderer #335

Refactor hbs renderer #335

Michael-F-Bryan commented Jun 15, 2017

steveklabnik commented Jun 15, 2017

Michael-F-Bryan commented Jun 15, 2017

Michael-F-Bryan commented Jun 15, 2017

budziq Jun 17, 2017 •

edited

Loading

Michael-F-Bryan Jun 17, 2017

budziq commented Jun 17, 2017

Michael-F-Bryan commented Jun 18, 2017

budziq commented Jun 18, 2017

azerupi commented Jun 18, 2017

budziq Jun 18, 2017

Michael-F-Bryan Jun 19, 2017

budziq Jun 18, 2017

Michael-F-Bryan Jun 19, 2017

Michael-F-Bryan Jun 20, 2017

budziq Jun 18, 2017

Michael-F-Bryan Jun 19, 2017

steveklabnik commented Jun 19, 2017

Michael-F-Bryan commented Jun 20, 2017

budziq commented Jun 20, 2017 •

edited

Loading

sunng87 commented Jun 21, 2017

Michael-F-Bryan commented Jun 22, 2017

sunng87 commented Jun 22, 2017

sunng87 commented Jun 22, 2017

azerupi commented Jun 22, 2017

azerupi Jun 23, 2017

Michael-F-Bryan Jun 24, 2017

Michael-F-Bryan commented Jun 24, 2017

azerupi commented Jun 24, 2017


		let mut content = String::new();

		let _source = File::open(destination.join(&ch.path.with_extension("html")))

Refactor hbs renderer #335

Refactor hbs renderer #335

Conversation

Michael-F-Bryan commented Jun 15, 2017

steveklabnik commented Jun 15, 2017

Michael-F-Bryan commented Jun 15, 2017

Michael-F-Bryan commented Jun 15, 2017

budziq Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budziq commented Jun 17, 2017

Michael-F-Bryan commented Jun 18, 2017

budziq commented Jun 18, 2017

azerupi commented Jun 18, 2017

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

steveklabnik commented Jun 19, 2017

Michael-F-Bryan commented Jun 20, 2017

budziq commented Jun 20, 2017 • edited Loading

sunng87 commented Jun 21, 2017

Michael-F-Bryan commented Jun 22, 2017

sunng87 commented Jun 22, 2017

sunng87 commented Jun 22, 2017

azerupi commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Jun 24, 2017

azerupi commented Jun 24, 2017

budziq Jun 17, 2017 •

edited

Loading

budziq commented Jun 20, 2017 •

edited

Loading