Skip to content

Initial Book loader and summary parsing #360

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

Closed
wants to merge 18 commits into from

Conversation

Michael-F-Bryan
Copy link
Contributor

This is the first step towards #359, creating a Loader type which is in charge of reading the source directory, parsing the SUMMARY.md, and then constructing an internal representation of the book.

This PR will refactor the existing summary parsing to return some Summary struct which indicates how the book is structured so that later steps can locate files and populate the book from disk.

@azerupi
Copy link
Contributor

azerupi commented Jun 24, 2017

Is the loader's only purpose to look for and parse the summary? In that case I would make the name more like: SummaryLoader or put it in the summary module.

Also, I feel like we might want the parsing method to not return immediately on the first syntax error. It might be more interesting to continue parsing to report multiple errors. I'm not sure how that is best handled, but since this exploratory work we can think a little more about this :)

@Michael-F-Bryan
Copy link
Contributor Author

I was thinking the Loader can parse the summary and then using that parsing result it'll load the book from disk.

So in very rough pseudo-code:

impl Loader {
  ...

  pub fn load(&self) -> Result<Book> {
    let summary = self.parse_summary().chain_err(|| "Invalid SUMMARY.md")?;
    
    let mut book = Book::new();

    for item in summary {
      match item {
        SummaryItem::Chapter(ch) => {
          let chapter = self.load_chapter()?;
          book.add_section(chapter);
        }
        SummaryItem::Separator => book.add_section(BookItem::Separator),
     }
    }
  }
  
  Ok(book)
}

Another idea would be to accumulate errors in some Vec while loading, then add a variant to the error-chain called "LoadError" which contains a Vec<Error> to indicate all the errors encountered while parsing.

@azerupi azerupi added the S-Work-in-progress Status: Work-in-progress label Jun 26, 2017
@Michael-F-Bryan
Copy link
Contributor Author

Oops... I think I accidentally did git merge instead of git rebase to pull in the latest changes from master. 346ddef upped the changes from ~300 lines and 2 files touched to ~30 files. I'll have to do a little cherry picking to fix up the history.

Anyways, I think the easiest thing for me to do to parse the summary is take advantage of pulldown_cmark and create a simple state machine which will continually munch new events from pulldown until there are none left, changing behaviour and incrementally building the Summary depending on what state you are in. After the Summary is created (it's just a tree representing of SUMMARY.md), you can walk it recursively and generate the corresponding BookItem node using the visitor pattern(e.g. a method may be fn visit_link(&self, node: Link) -> BookItem).

@budziq, @azerupi, and @steveklabnik, what are your thoughts on taking that approach compared to the current implementation which recursively walks the SUMMARY.md file, building up the MdBook all in one hit?

@Michael-F-Bryan Michael-F-Bryan force-pushed the loader branch 2 times, most recently from c713166 to bfc9112 Compare June 28, 2017 12:24
@Michael-F-Bryan
Copy link
Contributor Author

@azerupi, I've finally gotten things to the point where you can correctly parse a SUMMARY.md file and the full integration test passes! 😄

This PR is actually a lot smaller than it looks. There are only 362 executable lines of code in the loader module and something like 300-400 lines of tests. cargo kcov says the new summary parser has the largest number of lines covered by tests.

screenshot_2017-06-29_210528

Now I need to clean things up, deal with as many TODOs and FIXMEs as possible, and get other people to review so I know what bits can be improved.

@azerupi
Copy link
Contributor

azerupi commented Jun 29, 2017

Nice!
I will try to take a look at this later today :)

@Michael-F-Bryan Michael-F-Bryan changed the title [WIP] Initial Book loader and summary parsing Initial Book loader and summary parsing Jul 2, 2017
@Michael-F-Bryan
Copy link
Contributor Author

I'm not sure if its worth the effort to keep two separate PR's Why not incorporate all of the changes to #360 change it's title and close this one?

After @budziq's comment I think I'll closing this PR and will put merge #360 and #371 into a single pull request.

@Michael-F-Bryan Michael-F-Bryan deleted the loader branch July 3, 2017 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Work-in-progress Status: Work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants