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

mdBook forbids "empty chapters" since 0.1.0 #576

Closed
teiesti opened this issue Jan 24, 2018 · 24 comments
Closed

mdBook forbids "empty chapters" since 0.1.0 #576

teiesti opened this issue Jan 24, 2018 · 24 comments

Comments

@teiesti
Copy link

teiesti commented Jan 24, 2018

Until version 0.0.28, it was possible to have a SUMMARY.md like this:

# Summary

- [Real chapter](./real.md)
- [Empty chapter]()

While the "real chapter" was rendered and given a link in the summary on the left-hand side, the "empty chapter" was just given an entry in the summary without a link.

This behaviour was very helpful in lots of cases, e.g.

  • when the book was not yet complete or
  • to give the summary additional structure.

Since version 0.1.0, empty chapters are not longer supported. They result in the following error:

$ mdbook build
2018-01-24 16:08:20 [ERROR] (mdbook::utils): Error: Summary parsing failed
2018-01-24 16:08:20 [ERROR] (mdbook::utils): Caused By: There was an error parsing the numbered chapters
2018-01-24 16:08:20 [ERROR] (mdbook::utils): Caused By: Error at line 4, column 16: You can't have an empty link.

Is the new behaviour intended? If yes, is it possible to revert that decision since the old behaviour has practical benefits?

@Michael-F-Bryan
Copy link
Contributor

That's intended behaviour and was done in response to #566. The idea is that an empty link indicates you're linking to an invalid chapter.

We can always swap it from an error to a warning so it acts like a lint, then continue on past the link. What are your thoughts?

to give the summary additional structure.

I don't think links should be used for this purpose. It's probably a better idea to use separators --- or something which has semantic meaning. For example, I've seen The Book use headers to help break different sections of SUMMARY.md up.

@teiesti
Copy link
Author

teiesti commented Jan 24, 2018

What are your thoughts?

I have a certain use case in mind. Unfortunately, I can't show the book because its internal documentation... But I can explain the idea:

I have multiple locations with multiple machines and multiple services. Any machine and service got an FQDN as identifier. I have a book that acts as a reference for these FQDNs. The summary looks a bit like this:

  • Location 1
    • Machines
      • machine1.location1.example.com
      • machine2.location1.example.com
    • Services
      • service1.location1.example.com
      • service2.location1.example.com
  • Location 2
    • Machines
      • machine1.location2.example.com
    • Services
      • service1.location2.example.com

Any FQDN has a documentation and is therefore a chapter in the book. But it makes no sense for me to have documentation pages for entries like "Location 1" or "Machines". Their single purpose is to improve the structure on the left side of the page. (You may say they are empty chapters which are skipped when using the arrow keys.)

The second reasoning is: I really have a lot of FQDNs. Since I have not always used mdBook to render the documentation, not any FQDN is currently documented in mdBook -- I am still migrating. Those FQDNs have an entry in the summary but they are empty chapters that are skipped when moving through the documentation. Being forced to add a file for each of these "chapters" would simply bloat my repository at the current stage making it harder to track where work needs to be done. (Current approach is: No file available --> migration needed; file available --> migration complete or in progress)

For example, I've seen The Book use headers to help break different sections of SUMMARY.md up.

I really like that idea. But the obvious drawback is, that these headers are not represented in the rendered book. They are stripped by mdBook and therefore not really helpful.

@teiesti
Copy link
Author

teiesti commented Jan 24, 2018

I think, there are three ways to move on:

  1. Do nothing since I obviously abuse what mdBook was created for.
  2. Go back to the old behaviour and treat non existing links as "empty chapters" which are skipped when using navigation keys.
  3. Find another way to structure the summary that has an effect on the rendered book. (Structuring the summary only in the source make not really sense to me.)

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented Jan 24, 2018

I think the previous behaviour was actually an unintended, albeit useful, bug in the HTML renderer. The internals of mdbook have always been structured such that a Book contains a list of chapters, where each chapter corresponds to a source file on disk, possibly containing some sub-chapters (which may also contain sub-chapters, ad infinitum).

With that model in mind there isn't really any way to represent the idea of a "virtual" chapter, which then leads to issues of what to tie the sub-chapters to.

I know the idea of a virtual chapter which only exists for namespacing purposes has been asked about in the past, so I'm going to look into updating the SUMMARY.md parser and our Book structure.

In the meantime, I imagine the easiest workaround for you will be to roll back to the 0.0.28 version. Sorry we broke your workflow!

@teiesti
Copy link
Author

teiesti commented Jan 24, 2018

I know the idea of a virtual chapter which only exists for namespacing purposes has been asked about in the past, so I'm going to look into updating the SUMMARY.md parser and our Book structure.

That sounds great! Do you need help with the implementation?

@Michael-F-Bryan
Copy link
Contributor

More help is always welcome 😁

The process isn't overly difficult, it just requires updating several parts which are in different places in the codebase. Namely the summary parser, the load_summary_item() function and the HTML renderer.

@teiesti
Copy link
Author

teiesti commented Jan 24, 2018

I'll have a try on this -- but not today...

I think about adding a new variant Virtual to SummaryItem plus the changes you propose. But I am not completely sure about the name Virtual and whether I should include other things (like e.g. headings for TRPL) too...

Some advice?

@azerupi
Copy link
Contributor

azerupi commented Jan 26, 2018

@Michael-F-Bryan I just finished my exams and am going through all the things I missed in the last months, so I am little out of the loop and late to the party 😉 Excellent work on everything by the way!

I think the previous behaviour was actually an unintended, albeit useful, bug in the HTML renderer.

It was actually an intended feature. :)
It allowed you to indicate missing sections in the summary that were not written yet. For example, if you write a book but a couple of chapters are missing, you would use that to indicate what chapters you still plan to add without actually having them. In the summary, those chapters were grayed out and inaccessible.

I don't remember exactly how it was implemented, but I am sure it was not pretty, hence the reason it was probably mistaken for a side-effect.

@teiesti
Copy link
Author

teiesti commented Jan 26, 2018

It was actually an intended feature. :)

That explains why The Rust Programming Language uses that behavior in the appendix.

I don't remember exactly how it was implemented, but I am sure it was not pretty, hence the reason it was probably mistaken for a side-effect.

That sounds reasonable. I would suggest that we first revert the patch that removes the feature. Then, in a second step, we could think about how to implement it in a way that it is not mistaken for a side effect. Maybe we could also add a passage in the user guide.

Btw I've compared the current rendering result with TRPL. Seems that the line height in the summary is much higher now. Is that intended?

@Michael-F-Bryan
Copy link
Contributor

I would suggest that we first revert the patch that removes the feature. Then, in a second step, we could think about how to implement it in a way that it is not mistaken for a side effect.

I don't think its as easy as reverting a patch. This actually came about because we were restructuring internals to make things cleaner and formalize the Book structure more. The requirement that each chapter must have a valid parent, and each chapter must point at a file is a result of the way the Book type is structured... Reverting this change would effectively mean rolling back a decent chunk of the codebase 😞

I don't think we'd ever tested to make sure you can have an empty chapter for namespacing purposes, hence why it wasn't picked up until now.

For reference, the current structure looks something like this:

struct Book {
  sections: Vec<BookItem>,
}

enum BookItem {
  Separator,
  Chapter(Chapter),
}

struct Chapter {
  name: String,
  path: PathBuf,
  content: String,
  sub_items: Vec<BookItem>,
}

To do this properly, we'd need to add a VirtualChapter (may need some bikeshedding on the name) variant to the BookItem enum. I don't want to introduce a hacky solution because that's just adding technical debt and hurting the project's maintainability.

As an aside, it looks like this is very similar to #483.

@azerupi
Copy link
Contributor

azerupi commented Jan 27, 2018

To do this properly, we'd need to add a VirtualChapter (may need some bikeshedding on the name) variant to the BookItem enum.

Doing this is a breaking change no?

@Michael-F-Bryan
Copy link
Contributor

Doing this is a breaking change no?

I don't know, is it? Almost everyone uses the command line application and we should strive to prevent breaking changes when doing minor releases (although I screwed up by letting the configuration format change slip into 0.0.28 😞). In comparison, the internal library is only used by plugin creators (mainly myself at the moment), so I don't know what the best practice is with that. The mdbook crate itself and its internals are going to be unstable until we reach 1.0.

@steveklabnik, what are you usually meant to do when the main project is a program, but that binary uses a backing crate? I know the cargo crate doesn't have any rustdoc documentation and there aren't any real guarantees on the stability of the internals...

@steveklabnik
Copy link
Member

steveklabnik commented Jan 27, 2018

yeah, it's a versioning issue with cargo, basically. that is, the version is per-package, not per-crate.

that said, you're still pre-1.0, so this would be a "bump to 0.2" thing. no big deal.

oh, we also use mdbook as a library to build rustc. it wouldn't be particularly onerous to update though.

@azerupi
Copy link
Contributor

azerupi commented Jan 27, 2018

Yes I would bump the version whenever we do a breaking change, even if it is minor. :)
It is not something we should be afraid of.

@teiesti
Copy link
Author

teiesti commented Jan 27, 2018

although I screwed up by letting the configuration format change slip into 0.0.28

I actually appreciate that you've bumped to version to 0.1.0 because mdBook is now closer to the versioning scheme proposed by semver.

I would also bump the version (major, minor, patch -- whatever is necessary) whenever a change happens to the binary or the library since the package represents both.

Therefore, since this is a breaking change, the correct version would be 0.2.0 IMHO.

The mdbook crate itself and its internals are going to be unstable until we reach 1.0.

Exactly.

@donald-pinckney
Copy link
Contributor

Just to add one more data point, I was also relying on this feature. Specifically, I use it to represent chapters that are planned to be written, but are not yet:
screen shot 2018-03-17 at 11 43 22 am

For now I'll go back to 0.0.28

brookst added a commit to brookst/book that referenced this issue Mar 22, 2018
Update to mdbook circa 1.5. Convert json to toml, surpress incomplete chapters
(see: rust-lang/mdBook#576), and pull style sheet from latest theme.
@donald-pinckney
Copy link
Contributor

@teiesti @azerupi @Michael-F-Bryan whats the current status of implementation for this? There has been lots of great progress with mdbook, but I can't use any of it since I rely on this feature. I would be willing to help with implementation if it would help.

@teiesti
Copy link
Author

teiesti commented Oct 2, 2018

I've started to write a fix (#592) back in January but then got busy at university. @azerupi continued my work but I think he did not finish it. There has been no activity in the PR since March. I actually don't know what the current status is. I know that I still have no spare time to do this. Maybe you can take over from @azerupi?

@donald-pinckney
Copy link
Contributor

Ok, thanks for the update. I’ll try and take a look at what has been done.

@theduke
Copy link

theduke commented Dec 23, 2018

I'd also really need this feature.

@dvogt23
Copy link

dvogt23 commented Aug 12, 2019

I would like to switch from gitbook with auto-generated SUMMARY.md with gitbook-summary wich creates a summary with "empty" chapters like:

# Title

- EmptyChapter
  * [Subchap](http://link)
  * ...
- AnotherEmptyChap
  * ...

mdbook fails with that stucture because of EmptyChapter.

@JHBitencourt
Copy link

+1 for this feature. Running on mdbook v0.3.5 this is still not possible.

@azerupi
Copy link
Contributor

azerupi commented Mar 19, 2020

There is a PR open (#1153) that restores this behavior with the format of empty markdown links:

- [Empty chapter]()

It is in review, just a little more patience 🙂

@ehuss
Copy link
Contributor

ehuss commented May 17, 2020

Closed via #1153.

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 a pull request may close this issue.

9 participants