-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[rustdoc] Add new --book-location
option to add a link to associated guide and generate it if local
#139769
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
base: master
Are you sure you want to change the base?
Conversation
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
This comment has been minimized.
This comment has been minimized.
15e7374
to
1aefb85
Compare
This comment has been minimized.
This comment has been minimized.
1aefb85
to
bcb5ed2
Compare
This comment has been minimized.
This comment has been minimized.
bcb5ed2
to
cad5667
Compare
The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging. |
This comment has been minimized.
This comment has been minimized.
cad5667
to
3388b0b
Compare
This PR modifies cc @jieyouxu |
Finally fixed all tidy errors. :') |
src/librustdoc/config.rs
Outdated
@@ -229,6 +229,22 @@ impl fmt::Debug for Options { | |||
} | |||
} | |||
|
|||
#[derive(Clone, Debug)] | |||
pub(crate) enum PathOrURL { |
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.
pub(crate) enum PathOrURL { | |
pub(crate) enum PathOrUrl { |
to follow idiom, and to be consistent with the style in this crate
☔ The latest upstream changes (presumably #140336) made this pull request unmergeable. Please resolve the merge conflicts. |
Whoops, missed this in my notifications, sorry 😅
Any reason you're making it insta-stable? Seems like might as well go through the normal process of adding as an unstable flag first. |
It was already unstable. 😄 |
@@ -658,6 +658,14 @@ fn opts() -> Vec<RustcOptGroup> { | |||
"disable the minification of CSS/JS files (perma-unstable, do not use with cached files)", | |||
"", | |||
), | |||
opt( | |||
Unstable, |
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.
@camelid: Just checked and the option is indeed unstable. 😉
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.
Ah, I was just confused because you said it'd need an FCP, which I thought we only required for stabilizations.
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.
Hum, well it's a big change in rustdoc (as is, scope expansion), so better be sure everyone agrees with this direction.
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.
Looks good overall, but left some comments.
src/librustdoc/config.rs
Outdated
#[derive(Clone, Debug)] | ||
pub(crate) enum PathOrURL { | ||
Path(PathBuf), | ||
URL(String), |
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.
Ditto here with tshepang's suggestion from above.
@@ -658,6 +658,14 @@ fn opts() -> Vec<RustcOptGroup> { | |||
"disable the minification of CSS/JS files (perma-unstable, do not use with cached files)", | |||
"", | |||
), | |||
opt( | |||
Unstable, |
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.
Ah, I was just confused because you said it'd need an FCP, which I thought we only required for stabilizations.
src/librustdoc/lib.rs
Outdated
Err(error) => return Err(format!("failed to load book: {error:?}")), | ||
}; | ||
let output_dir = render_options.output.join("doc-book"); | ||
*book_dir = output_dir.join("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.
I don't like that this mutates the render_options.book_location
to record the new path of the book. I think it'd better to have two separate fields, one for the source location of the book and one for the target location (None if the source was a URL).
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.
Not sure to agree but not something I think is worth debating about so let's go. However: I'm gonna use PathOrUrl
because otherwise you'd need to use two types to get the information, which is suboptimal imo.
@@ -28,6 +28,7 @@ const LICENSES: &[&str] = &[ | |||
"Apache-2.0", | |||
"Apache-2.0/MIT", | |||
"BSD-2-Clause OR Apache-2.0 OR MIT", // zerocopy | |||
"CC0-1.0", // notify |
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.
Who's responsible for this license exceptions list? Feels like it'd be good to check that these exceptions are OK with someone who's more experienced in this area.
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.
I know the right person for this. cc @wesleywiser :)
@@ -0,0 +1,7 @@ | |||
//@ compile-flags: -Zunstable-options --book-location https://somewhere.world |
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.
We should have another (run-make) test that tests the mdbook integration, especially since that's where most of the complexity of this PR is.
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.
What do you want to test exactly?
## Generating link to guide | ||
|
||
You can generate a link to a guide or generate the guide with `mdbook` using the `--book-location` | ||
command line argument. It accepts either a URL or a path. If a path is provided, the book will | ||
be generated. |
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.
## Generating link to guide | |
You can generate a link to a guide or generate the guide with `mdbook` using the `--book-location` | |
command line argument. It accepts either a URL or a path. If a path is provided, the book will | |
be generated. | |
## Bundling or linking to a guide | |
You can generate a link to a guide or bundle an `mdbook` guide using the `--book-location` | |
command line argument. It accepts either a URL or a path. If a path is provided, the book will | |
be built with `mdbook`. |
3388b0b
to
d4fefe0
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #140726) made this pull request unmergeable. Please resolve the merge conflicts. |
7d007c9
to
eda5e44
Compare
Fixed merge conflict. |
This option adds the possibility to provide a path or a URL to the associated guide of this crate in the crate-level doc page. If the provided argument is a path, then the guide is generated using the
mdbook
crate.It looks like this:
You can give it a try here.
It'll need an FCP before getting merged.
Also I need to add tests when the argument is a path but I'll wait for the feature form to be final before doing it to prevent doing it more than once. 😄
cc @notriddle
r? @camelid