Fix heading links in nested pages #419

behnam · 2017-09-02T01:18:29Z

Plus fixing the whitespace chars not being replaced by hyphen.

Also expand tests for link creations, and add test for nested pages.

Fixes https://github.com/azerupi/mdBook/issues/416
Fixes https://github.com/azerupi/mdBook/issues/417

And, also fix some test warnings by moving non-test module files into their own directories.

azerupi · 2017-09-02T07:40:15Z

Thanks for the PR!
I did a very quick review pass and it looks very good for most parts! Travis fails because of rustup, but appveyor seems to run the tests and fail at check_correct_cross_links_in_nested_dir. I will do a more in depth review soon. :)

behnam · 2017-09-02T19:11:14Z

Uh, on windows the filepath needs to be changed to use forward-slash, instead of backslash. I'll submit an update.

behnam · 2017-09-02T20:47:53Z

Travis is completely broken right now. I filed this: https://github.com/azerupi/mdBook/issues/421

Michael-F-Bryan

Looks pretty good 👍

Breaking out the dummy book creation and helpers into two separate modules is a good idea, seeing as they're separate and the helpers module will probably expand as we add more tests.

Note to self: update #422 when this merges

Michael-F-Bryan · 2017-09-03T09:17:57Z

src/renderer/html_handlebars/hbs_renderer.rs

@@ -457,7 +458,7 @@ fn id_from_content(content: &str) -> String {
    let mut content = content.to_string();

    // Skip any tags or html-encoded stuff
-    let repl_sub = vec![
+    static REPL_SUB: &[&str] = &[


I'm assuming you meant const REPL_SUB ... here. Using static means it actually gets embedded in the binary and is typically only done for global mutable objects or if your hardware requires it (like the exception vector when writing a kernel).

I'd also pull it up to the top of the file and name it to something like HTML_ENCODE_BLACKLIST (feel free to give it a better name) so it's easy to update later on.

Ooop! Right.

Michael-F-Bryan · 2017-09-03T09:25:57Z

tests/rendered_output.rs

-        "first/nested.html",
-        "second.html",
-        "conclusion.html",
+        r#"href="intro.html""#,


Nice fix! Without the href=" this test wouldn't have actually been checking for links, just that the filenames are present, which means it's not actually testing what we want it to.

Michael-F-Bryan · 2017-09-03T09:26:56Z

src/lib.rs

@@ -100,6 +100,8 @@ pub use book::BookItem;
 pub use renderer::Renderer;

 /// The error types used through out this crate.
+// TODO: Drop after error_chain is fixed
+#[allow(unused_doc_comment)]


This should probably be pulled out into its own PR. error-chain has actually fixed this, so bumping the dependency version to 0.11.0-rc.2 would be sufficient.

yeah; I did this on one of my recent projects

Michael-F-Bryan · 2017-09-03T09:35:26Z

src/renderer/html_handlebars/hbs_renderer.rs

                let rendered = self.post_process(rendered,
-                    filename.file_name().unwrap().to_str().unwrap_or(""),
+                    &normalize_path(filepath.to_str()
+                        .expect(&format!("Bad file name: {}", filepath.display()))),


Is it a good idea to panic here? Another option may be to assume a safe default or return an error.

If we were to return an error, I imagine it'd look something like this:

filepath.to_str() .ok_or_else(Error::from("Bad file name: {}", filepath.display()))?`

Good point! I would expect non-Unicode file names would be caught somewhere earlier in the lib, but in case they are not, makes sense to return a nice error here.

Michael-F-Bryan · 2017-09-04T06:03:42Z

I just had another look through this PR... Are there any tests to make sure normalize_id() correctly translates titles containing spaces get converted to using hyphens?

It might also be useful to add an integration test in tests/rendered_output.rs to make sure the chapter title Chapter 1 gets a hyphenated id (i.e. <h1 id="chapter-1">Chapter 1</h1>).

Move non-test test module files into their own directories to prevent cargo from running them as tests. Then suppress the left-over warnings. Move *dummy book* code and data into a shared folder, and leave the rest of helper utilities (one function) in the original module.

Plus fixing the whitespace chars not being replaced by hyphen. Also expand tests for link creations, and add test for nested pages. Fixes <https://github.com/azerupi/mdBook/issues/416> Fixes <https://github.com/azerupi/mdBook/issues/417>

On the web, the normalized path separator is forward-slash (`/`), so we use the built-in `is_separator()` method to replace any path separator with the forward-slash, to ensure consistent output on unix and windows machines.

behnam · 2017-09-06T08:04:07Z

I just had another look through this PR... Are there any tests to make sure normalize_id() correctly translates titles containing spaces get converted to using hyphens?

Yes, the newly added "## Some Section" to the nested first pages actually tests SPACE being converted to HYPHEN, and in fact makes sure both href and id are set correctly.

I'm submitting a fix for the inline comments, and I think it would be good to land.

Addressing the review comments.

azerupi · 2017-09-06T10:27:34Z

Thank you for fixing the issues raised by @Michael-F-Bryan !
I will try to take a last look at this PR tonight to merge it ASAP.

steveklabnik · 2017-09-06T14:36:09Z

Awesome, and then we should cut a release.

azerupi · 2017-09-07T20:41:19Z

Thanks a lot for this PR @behnam!
I will publish a new release with the fixes in a couple of minutes.

Fix heading links in nested pages

behnam force-pushed the nested branch from 0079d5a to aa79ea6 Compare September 2, 2017 01:19

behnam mentioned this pull request Sep 2, 2017

Pull the cargo docs out of tree and convert them to mdBook rust-lang/cargo#4040

Closed

behnam force-pushed the nested branch 4 times, most recently from d7ac12d to a4533cf Compare September 2, 2017 20:41

Michael-F-Bryan reviewed Sep 3, 2017

View reviewed changes

steveklabnik mentioned this pull request Sep 5, 2017

update mdbook rust-lang/rust#44267

Closed

behnam added 3 commits September 6, 2017 00:52

Fix heading links in nested pages

956a5cc

Plus fixing the whitespace chars not being replaced by hyphen. Also expand tests for link creations, and add test for nested pages. Fixes <https://github.com/azerupi/mdBook/issues/416> Fixes <https://github.com/azerupi/mdBook/issues/417>

[renderer] Add normalize_path()

9994554

On the web, the normalized path separator is forward-slash (`/`), so we use the built-in `is_separator()` method to replace any path separator with the forward-slash, to ensure consistent output on unix and windows machines.

behnam force-pushed the nested branch from a4533cf to 9994554 Compare September 6, 2017 07:52

behnam force-pushed the nested branch from c19de8c to ddee839 Compare September 6, 2017 09:24

[renderer] Err on bad file names, instead of panic

ddee839

Addressing the review comments.

azerupi approved these changes Sep 7, 2017

View reviewed changes

azerupi merged commit f4513d3 into rust-lang:master Sep 7, 2017

behnam deleted the nested branch September 7, 2017 20:53

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

Merge pull request rust-lang#419 from behnam/nested

5eb57be

Fix heading links in nested pages

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix heading links in nested pages #419

Fix heading links in nested pages #419

behnam commented Sep 2, 2017

azerupi commented Sep 2, 2017

behnam commented Sep 2, 2017 •

edited

Loading

behnam commented Sep 2, 2017

Michael-F-Bryan left a comment

Michael-F-Bryan Sep 3, 2017

behnam Sep 6, 2017

Michael-F-Bryan Sep 3, 2017

Michael-F-Bryan Sep 3, 2017

steveklabnik Sep 5, 2017

Michael-F-Bryan Sep 3, 2017

behnam Sep 6, 2017

Michael-F-Bryan commented Sep 4, 2017

behnam commented Sep 6, 2017

azerupi commented Sep 6, 2017

steveklabnik commented Sep 6, 2017

azerupi commented Sep 7, 2017

Fix heading links in nested pages #419

Fix heading links in nested pages #419

Conversation

behnam commented Sep 2, 2017

azerupi commented Sep 2, 2017

behnam commented Sep 2, 2017 • edited Loading

behnam commented Sep 2, 2017

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Michael-F-Bryan Sep 3, 2017

Choose a reason for hiding this comment

behnam Sep 6, 2017

Choose a reason for hiding this comment

Michael-F-Bryan Sep 3, 2017

Choose a reason for hiding this comment

Michael-F-Bryan Sep 3, 2017

Choose a reason for hiding this comment

steveklabnik Sep 5, 2017

Choose a reason for hiding this comment

Michael-F-Bryan Sep 3, 2017

Choose a reason for hiding this comment

behnam Sep 6, 2017

Choose a reason for hiding this comment

Michael-F-Bryan commented Sep 4, 2017

behnam commented Sep 6, 2017

azerupi commented Sep 6, 2017

steveklabnik commented Sep 6, 2017

azerupi commented Sep 7, 2017

behnam commented Sep 2, 2017 •

edited

Loading