WIP: Add support for virtual chapters #592

teiesti · 2018-01-27T13:41:38Z

Beware: This is currently WIP.

This pull request introduces virtual chapters which do not correspondent to a file on disk. Virtual chapters can be used for namespacing purposes since they have an entry in the SUMMARY.md and the rendered navigation on the left side.

Work to be done:

Add a variant to BookItem and SummaryItem.
Modify the parser.
- Syntax A: - [Some virtual chapter]()
- Syntax B: - Some virtual chapter if desired (Design decision pending!)
Modify the renderer.
Add test cases.
Adjust the crate documentation.
Write a passage for the user guide.

Closes #483.
Closes #576.

azerupi · 2018-01-27T14:02:12Z

src/book/book.rs

+    /// The chapter's name.
+    pub name: String,
+    /// The chapter's contents.
+    pub content: String,


Is there any content in a virtual chapter?

I don't believe so. A virtual chapter just has a name, maybe a number, and a bunch of sub-items.

Is there any content in a virtual chapter?

This was actually a question somewhere else in a // TODO. I will remove the field.

azerupi · 2018-01-27T14:05:23Z

src/book/book.rs

+    Ok(ch)
+        /* TODO Open question: Do we really want to return a `Result<_>` here?
+         * Pro: Function would have almost signature as load_chapter()
+         * Con: We do not use it!


But we do use it. When you call load_summary_item recursively on the sub-items you return an error if any of the sub-items error.

You're right. I overlooked the ?-operator in line 284. Thanks!

Michael-F-Bryan · 2018-01-27T15:07:51Z

src/book/summary.rs

+///
+/// This is roughly the equivalent of `- Some virtual section`.
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct VirtualLink {


What would the difference be between a normal link and a virtual one? My understanding is you'd write a normal link as - [Some Chapter](./chapter.md) and a virtual link as - [Some Chapter]() (i.e. virtual chapters have no link in SUMMARY.md).

I think, that is an important design decision to make!

I propose that a user can write

- [Some Chapter](./chapter.md) for a link and

both - Some virtual chapter and - [Some virtual chapter]() for a virtual link.

Maybe, we could add a lint for - [Some virtual chapter]() but I would rather not do that.

For the moment, I've only implemented parsing - [Some virtual chapter](). - Some virtual chapter seems to be more tricky, therefore I would like to have a decision before going on... ;-)

teiesti · 2018-01-28T01:25:20Z

The parser seems to work -- at least for syntax A. Unfortunately, I have no idea where to start with the renderer. Can someone mentor me through?

teiesti · 2018-01-28T01:39:21Z

src/book/summary.rs

+                            last_link.nested_items = sub_items;
+                        },
+                        SummaryItem::Separator => unreachable!(),
+                    };


I don't really like this match because of the code duplication. Some ideas on how to make it nicer?

teiesti · 2018-01-28T01:41:02Z

src/book/summary.rs

+                            link.number = Some(number);
+                        },
+                        SummaryItem::Separator => panic!(),
+                    }


Another match. Same problem.

Michael-F-Bryan · 2018-01-28T08:18:28Z

You mentioned there are a lot of redundant match statements where we only care about dealing with a SummaryItem::VirtualLink or a SummaryItem::Link... What are your thoughts on removing the SummaryItem::VirtualLink variant and converting the path on a link to Option<PathBuf>. As far as I can see, the only difference between a virtual link and a normal one is the presence of a concrete path.

Michael-F-Bryan · 2018-01-28T08:24:46Z

I believe the main thing we need to do on the renderer side is update the toc and next/previous handlebars helpers. When the toc helper encounters a virtual chapter it should just emit a normal <li> with no link and just the chapter's name as the tag body (maybe also adding a CSS class?).

I think the navigation helpers should skip over a virtual chapter when you hit next/previous, although I'd have to have another look at the code to see how you are meant to do that.

teiesti · 2018-01-28T12:56:02Z

What are your thoughts on removing the SummaryItem::VirtualLink variant and converting the path on a link to Option. As far as I can see, the only difference between a virtual link and a normal one is the presence of a concrete path.

My thoughts are:

If we go for an Option<PathBuf>, we can also go for a PathBuf allowed to be empty. (This is, as I understand, the old solution.)
There is currently a correspondence between VirtualLink and VirtualChapter and Link and Chapter. I think, the solution you propose is less intuitive in this regard.

I am really unsure which solution is better. @azerupi What do you think?

azerupi · 2018-02-02T12:39:03Z

Yeah I think we should definitely have a separate item in the enum to represent those empty chapters. I am not up to date with the summary loading code however, so I don't have an opinion there.

mattico · 2018-02-05T01:06:37Z

src/book/book.rs

@@ -128,16 +128,24 @@ where
 pub enum BookItem {
    /// A nested chapter.
    Chapter(Chapter),
+    /// A nested virtual chapter.
+    VirtualChapter(VirtualChapter),
    /// A section separator.
    Separator,


While we're making breaking changes, it may be a good idea to add a variant:

// To make sure clients have a `_ =>` case #[doc(hidden)] __NonExhaustive,

to ensure client libraries can handle new enum variants.

Good idea. I will do that!

Michael-F-Bryan · 2018-02-18T08:21:12Z

Hey @teiesti, were you wanting any help with the rendering side of things?

teiesti · 2018-02-18T12:32:38Z

@Michael-F-Bryan: Yes.

By the way: Sorry for being inactive. I was busy learning for some university exams.

suhr · 2018-04-02T12:54:53Z

Status of this PR?

(If it stays inactive, I'll try to complete it myself. This feature is important)

teiesti · 2018-04-02T13:15:20Z

I am still waiting for help on the rendering side. If you want to give it a try, don't hesitate. Your work is appreciated... :-)

Michael-F-Bryan · 2018-04-02T13:35:11Z

@teiesti, what do you think you'll need help with from the rendering side? I believe it should be enough to update the make_data() function in src/renderer/html_handlebars/hbs_renderer.rs (creates the global context for template rendering) and then change the TOC helper so it'll render a virtual chapter correctly.

You'll probably want to write a test or two in either the TOC helper or as part of the integration test suite to make sure virtual chapters get added to the TOC but don't contain links to anywhere.

teiesti · 2018-04-02T23:17:08Z

I was thinking about this pull request again, and it seems that I won't have time for it soon. So, if anyone wants to take over, your welcome. Otherwise, I'll keep this open and try to complete my work when time is less rare. But this won't be in near future.

Sorry...

steveklabnik · 2018-04-03T15:39:50Z

(If it stays inactive, I'll try to complete it myself. This feature is important)

Agreed, though I'm also busy. @suhr do you want to give it a shot, as per @teiesti's last comment?

@teiesti thanks so much for all your work on this, I really appreciate it

suhr · 2018-04-04T20:56:49Z

Hm. It finds out, I have to deal with that 100 lines long toc.rs function. Which should have been fixed by #467, but still isn't.

How unfortunate.

This includes: - Use 'Self' instead of '<type name>' where possible. - Match enums in order of their variants.

This includes - a modification to parse_a_link() since parse_link() was modified and renamed to parse_item(), - a new test parse_a_virtual_link() and - the removal of an_empty_link_location_is_an_error() since an empty link location is parsed into a 'VirtualLink' from now on.

parse_nested_item() had to be adapted.

azerupi · 2018-04-13T23:07:21Z

I rebased this PR onto the latest master. The renderer actually still had support for "virtual chapters" from before. So I just added virtual chapters as chapters without a path when making the context for handlebars and it just worked as before.

I am sure it can be improved to make it more robust, but I propose that we merge this as is and improve it in further PRs.

Edit: I found a bug testing this. If the first chapter is a virtual chapter, the index.html will not be created. We should probably create an empty landing page only containing the TOC.

azerupi · 2018-04-14T16:30:04Z

I fixed the two remaining bugs, I think it is working correctly now.

azerupi · 2018-05-19T12:34:48Z

@Michael-F-Bryan could you review this when you have some time?
It should be mostly done for a first PR, there is maybe some polishing left to do. But I would like to merge this functionality soon, because one of my personal projetcs could use this. :)

Michael-F-Bryan

Looking at the changes, I believe we've covered everything necessary to add virtual chapters. @azerupi have you tried using this branch for your project?

When I get time I might come back and tidy up the parsing. The initial recursive descent approach gets the job done, but it can lead to some pretty confusing code and lots of redundant match statements. Since then I've gotten a lot better at parsing and we can probably feed the output from pulldown-cmark into lalrpop to make it all a lot nicer.

Michael-F-Bryan · 2018-05-20T04:39:40Z

src/renderer/html_handlebars/hbs_renderer.rs

            BookItem::Separator => {
                chapter.insert("spacer".to_owned(), json!("_spacer_"));
            }
+            _ => unimplemented!(),


Instead of a catch-all _ we should probably change it to __NonExhaustive => unreachable!(), that way we can't accidentally forget to update this match statement when the BookItem enum gets updated.

Michael-F-Bryan · 2018-05-20T10:14:13Z

src/book/summary.rs

+
+                            link.number = Some(number);
+                        },
+                        SummaryItem::Separator => panic!(),


For consistency with the previous SummaryItem::Separator (line 438) , this should probably be changed to unreachable!().

Michael-F-Bryan · 2018-05-20T10:15:16Z

src/book/summary.rs

-        .filter_map(|(i, item)| item.maybe_link_mut().map(|l| (i, l)))
+        .filter(|&(_, ref item)| {
+            match **item {
+                SummaryItem::Link(_) => true,


This can actually be condensed a bit.

SummaryItem::Link(_) | SummaryItem::VirtualLink(_) => true, SummaryItem::Separator => false,

Michael-F-Bryan · 2018-05-20T10:16:32Z

src/book/summary.rs

+
+        let href = match parser.stream.next() {
+            Some(Event::Start(Tag::Link(href, _))) => href,
+            other => panic!("Unreachable, {:?}", other),


Did you mean unreachable!()?

I suppose it has been written like this to offer more context in case the test fails.

azerupi · 2018-05-20T12:46:34Z

Thanks for the review!
I will try address the comments later today.

have you tried using this branch for your project?

I have, it works great on the books I tried it on :)

Dylan-DPC-zz · 2019-05-07T22:38:25Z

@teiesti do you want to continue with this? else it will be a good idea to close it and open a new PR when you feel like doing it again. Thanks for contributing

teiesti · 2019-05-09T07:58:40Z

@Dylan-DPC I think, it was way overdue to close this PR, cause it was laying around bitrotting. I still want to finish this work but in fact I won't have time anytime soon.

azerupi reviewed Jan 27, 2018

View reviewed changes

Michael-F-Bryan reviewed Jan 27, 2018

View reviewed changes

teiesti commented Jan 28, 2018

View reviewed changes

src/book/summary.rs

link.number = Some(number);

},

SummaryItem::Separator => panic!(),

}

Copy link

Author

teiesti Jan 28, 2018

Choose a reason for hiding this comment

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

Another match. Same problem.

mattico reviewed Feb 5, 2018

View reviewed changes

Michael-F-Bryan changed the title ~~Add support for virtual chapters~~ WIP: Add support for virtual chapters Feb 18, 2018

teiesti added 11 commits April 14, 2018 00:28

Add variant 'VirtualChapter' to 'BookItem'

672e102

Add variant 'VirtualLink' to 'SummaryItem'

bae55fb

Reduce technical depts

598c8e3

This includes: - Use 'Self' instead of '<type name>' where possible. - Match enums in order of their variants.

Extend load_summary_item() in order to support virtual chapters

7397e63

Remove unnecessary field 'content' in 'VirtualChapter'

bcab4e2

Remove resolved open question

95995cb

Implement parsing '- [Some virtual chapter]()' as 'VirtualLink'

6205658

Fix example in the documentation

3c25b3f

Fix 1640e5990bb36b187a9f31590546347f249d3343

c9605a0

parse_nested_item() had to be adapted.

Rename parse_item() to parse_linklike()

4a3d994

teiesti and others added 5 commits April 14, 2018 00:32

Add variant __NonExhaustive to BookItem

ed8fd38

remove unused import

40b98d6

fix missing parameters

9e5d5bb

Make virtual chapters work with the renderer

82197b7

fix test missing enum variant

ee45e5f

azerupi added 2 commits April 14, 2018 02:20

Generate empty index.html when first chapter is virtual

a5e7421

Fix bug where sub_items of virtual chapters were not visited

3990398

Michael-F-Bryan reviewed May 20, 2018

View reviewed changes

teiesti mentioned this pull request Oct 2, 2018

mdBook forbids "empty chapters" since 0.1.0 #576

Closed

Dylan-DPC-zz closed this May 7, 2019

suhr mentioned this pull request May 11, 2019

Change the doc structure tox-rs/book#15

Closed

flexibeast mentioned this pull request Jul 22, 2020

Allow for non-link summary items #483

Open

WIP: Add support for virtual chapters #592

WIP: Add support for virtual chapters #592

Conversation

teiesti commented Jan 27, 2018 • edited Loading

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

teiesti commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Jan 28, 2018

Michael-F-Bryan commented Jan 28, 2018

teiesti commented Jan 28, 2018

azerupi commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Feb 18, 2018

teiesti commented Feb 18, 2018

suhr commented Apr 2, 2018

teiesti commented Apr 2, 2018

Michael-F-Bryan commented Apr 2, 2018

teiesti commented Apr 2, 2018

steveklabnik commented Apr 3, 2018

suhr commented Apr 4, 2018

azerupi commented Apr 13, 2018 • edited Loading

azerupi commented Apr 14, 2018

azerupi commented May 19, 2018

Michael-F-Bryan left a comment • edited Loading

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

azerupi commented May 20, 2018

Dylan-DPC-zz commented May 7, 2019

teiesti commented May 9, 2019

teiesti commented Jan 27, 2018 •

edited

Loading

azerupi commented Apr 13, 2018 •

edited

Loading

Michael-F-Bryan left a comment •

edited

Loading