Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip HTML nodes in SUMMARY.md #1437

Merged
merged 2 commits into from
May 4, 2021

Conversation

tatsuya6502
Copy link
Contributor

This pull request changes mdBook 0.4.x to skip any HTML node (such as HTML comment) in SUMMARY.md.

Here is an example SUMMARY.md from rust-lang-ja/book-ja repository. This repository is for a Japanese translation of "The Rust Programming Language" book.

 1: <!--
 2: # The Rust Programming Language
 3: -->
 4: # Rustプログラミング言語
 5:
 6: <!--
 7: [The Rust Programming Language](title-page.md)
 8: [Foreword](foreword.md)
 9: [Introduction](ch00-00-introduction.md)
10: -->
11: [The Rust Programming Language 日本語版](title-page.md)
12: [まえがき](foreword.md)
13: [導入](ch00-00-introduction.md)
14:
15: <!--
16: ## Getting started
17: -->
18: ## 事始め
19: 
20: <!--
21: - [Getting Started](ch01-00-getting-started.md)
22:     - [Installation](ch01-01-installation.md)
23:     - [Hello, World!](ch01-02-hello-world.md)
24:     - [Hello, Cargo!](ch01-03-hello-cargo.md)
25: -->
26: - [事始め](ch01-00-getting-started.md)
27:     - [インストール](ch01-01-installation.md)
...

(We are keeping the original English texts in HTML comments to track future updates in the original)

Without this change, mdBook 0.4.x will fail as the followings:

$ mdbook build
[ERROR] (mdbook::utils): Error: Summary parsing failed
[ERROR] (mdbook::utils): 	Caused By: There was an error parsing the suffix chapters
[ERROR] (mdbook::utils): 	Caused By: failed to parse SUMMARY.md line 26, column 1: 
Suffix chapters cannot be followed by a list

Note that mdBook 0.3.7 can parse the same SUMMARY.md without problem.

@ehuss
Copy link
Contributor

ehuss commented Jan 20, 2021

Thanks for the PR! It looks like the problem is a little more complex thank how first is handled. From what I can see, the problem is that parse_title is exiting early because of the comment appearing before the H1 tag. Everything after that gets a little confused because it is out of order.

I think at a minimum parse_title would need to skip over leading Html events.

As for the first handling, do you know why it is there? Under what circumstances does a paragraph event happen as the first event in parse_numbered? Why do you handle html_node differently than other things that are ignored?

- Revert changes in parse_numbered(). They were unnecessary.
@tatsuya6502
Copy link
Contributor Author

I think at a minimum parse_title would need to skip over leading Html events.

As for the first handling, do you know why it is there? Under what circumstances does a paragraph event happen as the first event in parse_numbered? Why do you handle html_node differently than other things that are ignored?

@ehuss — Thanks for your feedback. I realized that my change in parse_numbered was not necessary, so I reverted them in my latest commit (#30ce7e79). I verified that the unit test skip_html_comments (added by my first commit) still passes.

When I started to work on this PR, I changed parse_numbered first and it solved some of my problems with SUMMARY.md. But then I realized parse_title needed a change too. So I changed parse_title but I did not check if my change in parse_numbered was still necessary. That was why my first commit still had the both changes.

Please review this PR again and let me know if you have any questions.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@ehuss ehuss merged commit 84b3b72 into rust-lang:master May 4, 2021
@ehuss
Copy link
Contributor

ehuss commented May 10, 2021

@y-yu, sure! 0.4.8 should be published now.

@y-yu
Copy link

y-yu commented May 14, 2021

Thanks!👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants