Book representation - Attempt 3

[Michael-F-Bryan]

Due to excessive bitrot, I've decided to start #409 over again on top of master instead of trying to do a really complicated git rebase and most probably accidentally clobbering a bunch of changes made since the branch was created. I made sure to steal all the good bits from that PR though!

This PR makes a lot of breaking changes!

Due to how central our book's internal representation is to the library, I've ended up touching a lot more stuff than I'd like and was forced to do a lot of drive-by refactoring 😞

Changes introduced in this PR:

• Completely re-implemented the SUMMARY.md parser to use the pulldown_cmark crate instead of a custom Markdown parser
• Completely reimplemented the Book (internal representation of a book and its contents) and the BookItem so that it's effectively a list of BookItems, where the numbered sections can also contain a bunch of BookItems.
• Refactored MDBook::init() so that instead of being a method which will initialise the MDBook, it returns a BookBuilder which you can use to customise how the book and its directory structure will be created
• Removed the MDBook::new() constructor and replaced it with a (fallible) MDBook::load() constructor which will immediately try to load the book from the provided source directory
  • This is part of a larger refactoring which ensures that you can only ever have a MDBook in a valid state. Previously you could load the MDBook in an indeterminate state and would then have to manually call init() or parse_config() to make sure things are okay. That still didn't ensure the SUMMARY.md file in src/ was valid (or even exists!) and that we could load the book.
  • Calling build() on the BookBuilder will generate all the files on disk, then immediately load the book and give you a MDBook
• Removes the create_missing feature which will automatically create any files from SUMMARY.md which don't exist
  • Although super convenient, this complicates the book parsing process and adds unexpected side-effects (loading a book should only read things from the filesystem)
  • It's also pretty magical (who would have thought that reading the Book from disk would also generate a bunch of files?)
  • @azerupi was a big fan of this feature, so I'd like to hear his thoughts on removing this.

Almost all of the above changes are backwards incompatible and will require a new minor release.

[Michael-F-Bryan]

@budziq this still has a fair amount of work to do to clean everything up and iron out the bugs, but it's starting to look the way I'd like. It was easier to start over than to try and solve several months worth of merge conflicts.

All tests currently pass, plus the dummy_book builds and renders correctly. The example book doesn't currently build because the renderer implicitly assumes it knows where the source files are on disk and uses that when resolving playpen links instead of using just the information provided by the in-memory Book (I'm pretty sure that's what the issue is anyway).

I'll try to fix the issue, although the root cause is that our HTML renderer is intimately coupled to the internals of MDBook. The long term solution for this should just "fall out" when we start adding alternate renderers because it'll force us to decouple everything and create a proper API.

[Michael-F-Bryan]

@budziq and @steveklabnik, this PR has implemented most of the stuff it needs to so how should I go about the whole review process?

It's a pretty large set of changes so it's probably not a good idea for me to merge this without getting someone else to have a skim through. But at the same time I'm quite conscious of the fact that you guys are quite busy and probably won't have time to do the review yourselves.

Can you think of anyone else who's familiar with the internals of mdbook and would able to give this a review?

[Michael-F-Bryan]

I've finally had time to sit down and work on this PR and so far I'm pretty happy with how it's turned out.

Something to note is one of the "breaking changes" I mentioned.

• Removes the create_missing feature which will automatically create any files from SUMMARY.md which don't exist
  • Although super convenient, this complicates the book parsing process and adds unexpected side-effects (loading a book should only read things from the filesystem)
  • It's also pretty magical (who would have thought that reading the Book from disk would also generate a bunch of files?)
  • @azerupi was a big fan of this feature, so I'd like to hear his thoughts on removing this.

@cspiegel, you recently did some work to move the --no-create flag to the configuration file. I don't want to just go and delete something which someone put a lot of hard work into, so I'd like to hear your thoughts on this? Do you think we should keep the create_missing feature, or would it be alright to remove it?

Also, @steveklabnik and @budziq, do you guys find this feature being used a lot in The Book and The Rust Cookbook, respectively?

[azerupi]

azerupi was a big fan of this feature, so I'd like to hear his thoughts on removing this.

Indeed, I like this feature because it makes for a smoother workflow. I know for a fact that it made at least one other person happy, because he mentioned it on twitter 😋 In my opinion, it would be a pity to see it go away.

[Michael-F-Bryan]

I know for a fact that it made at least one other person happy, because he mentioned it on twitter

That's a good enough reason for me :) I'll see if I can implement it in a cleaner way than before, that was the big factor making me question whether to keep it.

[Michael-F-Bryan]

Aaaannnddd it's back (ace0b51).

This time round I implemented it as a pass done after reading the summary but before we load the actual book. Previously we had to thread create_missing all the way down into the summary file parser, where it'd create a missing file if it found one while parsing SUMMARY.md. I feel like this is a much cleaner way of doing things, plus, because we're passing in part of the Config it leaves a lot of room for later if we want to tweak the book loading process.

[cspiegel]

@Michael-F-Bryan

@cspiegel, you recently did some work to move the --no-create flag to the configuration file. I don't want to just go and delete something which someone put a lot of hard work into, so I'd like to hear your thoughts on this? Do you think we should keep the create_missing feature, or would it be alright to remove it?

Looks like it's already been decided to keep it, but just for reference, I have no qualms about it being pulled out wholesale, if that's the direction things ultimately go.

[Michael-F-Bryan]

I think I'm going to merge this soon. There are a couple places which may need some more work, but the overall design is pretty solid (decoupling Book from MDBook, making it impossible to have a MDBook in an inconsistent state, etc) and 90% of it is done.

This is more meant as a refactoring of the current system and underlying infrastructure to make it less coupled and easier to maintain. I've added a lot of tests, in particular using the SUMMARY.md from rust-by-example and The Book to make sure no regressions are introduced.

Now that the internal representation of a Book is easier to work with, we can start on things like alternate backends or plugins.

cc: @frewsxcv, @budziq, @steveklabnik - I'm not sure if any of you guys will have time to do a full review, but it'd be nice to hear your thoughts. Skimming this issue's description and checking out a couple of the integration tests or rustdoc documentation will probably be enough to get you up to speed.

[azerupi]

I took a quick look and it looks good. I did not read everything, but I liked what I saw. Great job! 🎉

[Michael-F-Bryan]

Cheers! I've started working on an alternate backend and found the new Book representation, along with the refactored Config, made the entire process super easy.