-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add index preprocessor #685
Add index preprocessor #685
Conversation
README.md is a de facto index file in markdown-based documentation. Hence, we respect to README.md and convert it into index.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weihanglo I really like the idea you're proposing here!
We probably want to add an integration test to make sure the README.md
is actually rendered to index.html
(look at the other tests in tests/rendered_output.rs
for inspiration), but other than that I'm really happy with this PR.
src/preprocess/index.rs
Outdated
fn run(&self, _ctx: &PreprocessorContext, book: &mut Book) -> Result<()> { | ||
book.for_each_mut(|section: &mut BookItem| { | ||
if let BookItem::Chapter(ref mut ch) = *section { | ||
if ch.path.file_name().unwrap_or_default() == "README.md" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, do you think it's worth also checking for something like readme.md
or README
? So using the file stem instead and a case insensitive match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I almost forgot there is a file_stem
method can use. Sounds great!
I have some concerns with opt-in this feature by default:
Should we need to inspect this issue? Or just leave the decision for users. Let users opt-out the preprocessor or reorganize their book structure manually. We can also check out how gitbook did on this issue. I tried and it seems that if both |
I'd probably emit a warning (if possible) and then not apply the automatic conversion.
I think this is going to happen quite rarely in practice. Most people have their books in a subdirectory instead of the root directory, so the chance of having I'd prefer for this preprocessor to be included by default, otherwise it'll be hard to discover and never get used. |
As for file name conflict issue, I decided to convert all README.md and index.md without regarding any pitfall. Just as the same behavior you metioned above, it only emit some warnings to inform users to fix it. |
I like the integration tests and the check for matching different variants of This PR looks about ready to merge. If you've got time, would you be able to make a follow-up PR which documents the |
@Michael-F-Bryan |
* Add index preprocessor README.md is a de facto index file in markdown-based documentation. Hence, we respect to README.md and convert it into index.html. * Fix warning for unused variables * Update tests for config * Match file stem case-insensitively for IndexPreprocessor * Add tests for IndexPreprocessor * Update book example to fit index preprocessor
The PR is for
Add new preprocessor to convert README.md to index.md.
Intent
README.md is a de facto index file in markdown-based documentation. Many version control service such as GitHub, Gitlab and Bitbucket render README.md as index files while browsering source code on their web client. Hence, we may respect to README.md and convert it into index.md which would then convert into index.html.
I think this behavior is what people expect nowadays. It would be nice to include this feature.