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

Add {{#shiftinclude}} command #2333

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

daviddrysdale
Copy link

Allows included code to be shifted left or right by a specified amount, or to be automatically left-shifted so that common leading whitespace is stripped.

Uses a new command name #shiftinclude to avoid changing the behaviour of any existing books.

There are few open issues1 that this might help with :

There are also a few open PRs that this might overtake/replace/combine-with:

Footnotes

  1. I confess that I implemented this without looking at the current open issues/pull requests.

Syntax is the same as {{#include}} except with a shift value and colon
before the remaining arguments, e.g.

  {{#include -2:somefile.rs:myanchor}}

A positive value for the shift prepends spaces to each line.

A negative value for the shift removes chars from the beginning of each
line (including non-whitespace chars, although this will emit an error
log).
@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Mar 2, 2024
As well as allowing explicitly-specified shift amounts, also
support an "auto" option that strips common leftmost whitespace
from an inclusion.
daviddrysdale added a commit to daviddrysdale/mdbook-shiftinclude that referenced this pull request Mar 30, 2024
Taken from suggested pull request for upstream:
rust-lang/mdBook#2333
daviddrysdale added a commit to daviddrysdale/mdbook-shiftinclude that referenced this pull request Mar 30, 2024
Taken from suggested pull request for upstream:
rust-lang/mdBook#2333
daviddrysdale added a commit to daviddrysdale/mdbook-shiftinclude that referenced this pull request Mar 30, 2024
Taken from suggested pull request for upstream:
rust-lang/mdBook#2333
@daviddrysdale
Copy link
Author

Given that this looks unlikely to be included in the main mdBook code, I've pulled the functionality out into a separate mdbook-shiftinclude preprocessor.

Shift::Auto
} else {
let shift: isize = param0.parse().unwrap_or_else(|e| {
log::error!("failed to parse shift amount: {e:?}");
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should probably return an Option, so that the shiftinclude directive isn't parsed at all when there's an invalid shift amount.

The problem with this setup is that it isn't carrying the filename:linenumber in the error message, so finding the failed shiftinclude is really hard. If it refuses to parse it entirely, instead of substituting zero, it'll be easier to find where it failed by looking for the literal text {{#shiftinclude in your book.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, makes sense, thanks.

Done.

if l.chars().take(skip).any(|c| !c.is_whitespace()) {
log::error!("left-shifting away non-whitespace");
}
let rest = l.chars().skip(skip).collect::<String>();
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as there's no easy way to report the line number where the error is actually occurring, it would probably be good to avoid left shifting away non-whitespace characters. That way, even if the indentation comes out weird, it won't come out broken.

Suggested change
let rest = l.chars().skip(skip).collect::<String>();
let rest = l.chars()
.take_while(|c| c.is_whitespace())
.skip(skip)
.collect::<String>();

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I was in two minds as to whether left-shift should remove non-whitespace or not. Done

Rather than defaulting to a shift of 0, instead just skip over a 
shiftinclude command with an invalid shift specifier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants