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

librustdoc: Use correct heading levels. #89506

Merged
merged 8 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions src/librustdoc/externalfiles.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::html::markdown::{ErrorCodes, IdMap, Markdown, Playground};
use crate::html::markdown::{ErrorCodes, HeadingOffset, IdMap, Markdown, Playground};
use crate::rustc_span::edition::Edition;
use std::fs;
use std::path::Path;
Expand Down Expand Up @@ -39,14 +39,32 @@ impl ExternalHtml {
let bc = format!(
"{}{}",
bc,
Markdown(&m_bc, &[], id_map, codes, edition, playground).into_string()
Markdown {
content: &m_bc,
links: &[],
ids: id_map,
error_codes: codes,
edition,
playground,
heading_offset: HeadingOffset::H2,
GuillaumeGomez marked this conversation as resolved.
Show resolved Hide resolved
}
.into_string()
);
let ac = load_external_files(after_content, diag)?;
let m_ac = load_external_files(md_after_content, diag)?;
let ac = format!(
"{}{}",
ac,
Markdown(&m_ac, &[], id_map, codes, edition, playground).into_string()
Markdown {
content: &m_ac,
links: &[],
ids: id_map,
error_codes: codes,
edition,
playground,
heading_offset: HeadingOffset::H2,
}
.into_string()
);
Some(ExternalHtml { in_header: ih, before_content: bc, after_content: ac })
}
Expand Down
72 changes: 55 additions & 17 deletions src/librustdoc/html/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,19 @@
//! extern crate rustc_span;
//!
//! use rustc_span::edition::Edition;
//! use rustdoc::html::markdown::{IdMap, Markdown, ErrorCodes};
//! use rustdoc::html::markdown::{HeadingOffset, IdMap, Markdown, ErrorCodes};
//!
//! let s = "My *markdown* _text_";
//! let mut id_map = IdMap::new();
//! let md = Markdown(s, &[], &mut id_map, ErrorCodes::Yes, Edition::Edition2015, &None);
//! let md = Markdown {
//! content: s,
//! links: &[],
//! ids: &mut id_map,
//! error_codes: ErrorCodes::Yes,
//! edition: Edition::Edition2015,
//! playground: &None,
//! heading_offset: HeadingOffset::H2,
//! };
//! let html = md.into_string();
//! // ... something using html
//! ```
Expand Down Expand Up @@ -47,6 +55,8 @@ use pulldown_cmark::{
#[cfg(test)]
mod tests;

const MAX_HEADER_LEVEL: u32 = 6;

/// Options for rendering Markdown in the main body of documentation.
pub(crate) fn main_body_opts() -> Options {
Options::ENABLE_TABLES
Expand All @@ -65,20 +75,33 @@ pub(crate) fn summary_opts() -> Options {
| Options::ENABLE_SMART_PUNCTUATION
}

#[derive(Debug, Clone, Copy)]
pub enum HeadingOffset {
H1 = 0,
Copy link
Member

Choose a reason for hiding this comment

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

This looks incorrect to me. H1 should equal 1, no? Otherwise, it seems we'd get <h0>...</h0>.

Copy link
Member

@camelid camelid Oct 6, 2021

Choose a reason for hiding this comment

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

Also, I'd much rather we have a fn level(&self) -> u32 method than relying on enum casting. Casting makes it harder to see where the enum's being converted to a number.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, 0 is correct. The numbers there are added to the level specified in the Markdown file, so “# ghir” gets turned into h1, because it’s (1 + 0).

Copy link
Contributor Author

@yaymukund yaymukund Oct 7, 2021

Choose a reason for hiding this comment

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

It's an offset, so it gets added to the normal heading level. e.g. HeadingOffset::H2 means you add 1 level so # this is an h2 and ## this is an h3. Maybe HeadingStart would be a clearer name?

I agree with you on fn level(&self).

Edit: oops, i did not see you had already responded 😅

Copy link
Member

Choose a reason for hiding this comment

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

I see, that makes sense. Personally, I'd think it'd be clearer to use struct HeadingOffset { offset: u32 } or something like that. Having H1...H6 variants makes it look like it represents <h1> through <h6>.

H2,
H3,
H4,
H5,
H6,
}

/// When `to_string` is called, this struct will emit the HTML corresponding to
/// the rendered version of the contained markdown string.
pub struct Markdown<'a>(
pub &'a str,
pub struct Markdown<'a> {
pub content: &'a str,
/// A list of link replacements.
pub &'a [RenderedLink],
pub links: &'a [RenderedLink],
/// The current list of used header IDs.
pub &'a mut IdMap,
pub ids: &'a mut IdMap,
/// Whether to allow the use of explicit error codes in doctest lang strings.
pub ErrorCodes,
pub error_codes: ErrorCodes,
/// Default edition to use when parsing doctests (to add a `fn main`).
pub Edition,
pub &'a Option<Playground>,
);
pub edition: Edition,
pub playground: &'a Option<Playground>,
/// Offset at which we render headings.
/// E.g. if `heading_offset: HeadingOffset::H2`, then `# something` renders an `<h2>`.
pub heading_offset: HeadingOffset,
}
/// A tuple struct like `Markdown` that renders the markdown with a table of contents.
crate struct MarkdownWithToc<'a>(
crate &'a str,
Expand Down Expand Up @@ -489,11 +512,17 @@ struct HeadingLinks<'a, 'b, 'ids, I> {
toc: Option<&'b mut TocBuilder>,
buf: VecDeque<SpannedEvent<'a>>,
id_map: &'ids mut IdMap,
heading_offset: HeadingOffset,
}

impl<'a, 'b, 'ids, I> HeadingLinks<'a, 'b, 'ids, I> {
fn new(iter: I, toc: Option<&'b mut TocBuilder>, ids: &'ids mut IdMap) -> Self {
HeadingLinks { inner: iter, toc, buf: VecDeque::new(), id_map: ids }
fn new(
iter: I,
toc: Option<&'b mut TocBuilder>,
ids: &'ids mut IdMap,
heading_offset: HeadingOffset,
) -> Self {
HeadingLinks { inner: iter, toc, buf: VecDeque::new(), id_map: ids, heading_offset }
}
}

Expand Down Expand Up @@ -530,6 +559,7 @@ impl<'a, 'b, 'ids, I: Iterator<Item = SpannedEvent<'a>>> Iterator
self.buf.push_front((Event::Html(format!("{} ", sec).into()), 0..0));
}

let level = std::cmp::min(level + (self.heading_offset as u32), MAX_HEADER_LEVEL);
self.buf.push_back((Event::Html(format!("</a></h{}>", level).into()), 0..0));

let start_tags = format!(
Expand Down Expand Up @@ -1005,7 +1035,15 @@ impl LangString {

impl Markdown<'_> {
pub fn into_string(self) -> String {
let Markdown(md, links, mut ids, codes, edition, playground) = self;
let Markdown {
content: md,
links,
mut ids,
error_codes: codes,
edition,
playground,
heading_offset,
} = self;

// This is actually common enough to special-case
if md.is_empty() {
Expand All @@ -1026,7 +1064,7 @@ impl Markdown<'_> {

let mut s = String::with_capacity(md.len() * 3 / 2);

let p = HeadingLinks::new(p, None, &mut ids);
let p = HeadingLinks::new(p, None, &mut ids, heading_offset);
let p = Footnotes::new(p);
let p = LinkReplacer::new(p.map(|(ev, _)| ev), links);
let p = TableWrapper::new(p);
Expand All @@ -1048,7 +1086,7 @@ impl MarkdownWithToc<'_> {
let mut toc = TocBuilder::new();

{
let p = HeadingLinks::new(p, Some(&mut toc), &mut ids);
let p = HeadingLinks::new(p, Some(&mut toc), &mut ids, HeadingOffset::H1);
let p = Footnotes::new(p);
let p = TableWrapper::new(p.map(|(ev, _)| ev));
let p = CodeBlocks::new(p, codes, edition, playground);
Expand Down Expand Up @@ -1077,7 +1115,7 @@ impl MarkdownHtml<'_> {

let mut s = String::with_capacity(md.len() * 3 / 2);

let p = HeadingLinks::new(p, None, &mut ids);
let p = HeadingLinks::new(p, None, &mut ids, HeadingOffset::H1);
let p = Footnotes::new(p);
let p = TableWrapper::new(p.map(|(ev, _)| ev));
let p = CodeBlocks::new(p, codes, edition, playground);
Expand Down Expand Up @@ -1295,7 +1333,7 @@ crate fn markdown_links(md: &str) -> Vec<MarkdownLink> {
// There's no need to thread an IdMap through to here because
// the IDs generated aren't going to be emitted anywhere.
let mut ids = IdMap::new();
let iter = Footnotes::new(HeadingLinks::new(p, None, &mut ids));
let iter = Footnotes::new(HeadingLinks::new(p, None, &mut ids, HeadingOffset::H1));

for ev in iter {
if let Event::Start(Tag::Link(kind, dest, _)) = ev.0 {
Expand Down
52 changes: 34 additions & 18 deletions src/librustdoc/html/markdown/tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{find_testable_code, plain_text_summary, short_markdown_summary};
use super::{ErrorCodes, IdMap, Ignore, LangString, Markdown, MarkdownHtml};
use super::{ErrorCodes, HeadingOffset, IdMap, Ignore, LangString, Markdown, MarkdownHtml};
use rustc_span::edition::{Edition, DEFAULT_EDITION};

#[test]
Expand Down Expand Up @@ -147,74 +147,90 @@ fn test_lang_string_tokenizer() {
fn test_header() {
fn t(input: &str, expect: &str) {
let mut map = IdMap::new();
let output =
Markdown(input, &[], &mut map, ErrorCodes::Yes, DEFAULT_EDITION, &None).into_string();
let output = Markdown {
content: input,
links: &[],
ids: &mut map,
error_codes: ErrorCodes::Yes,
edition: DEFAULT_EDITION,
playground: &None,
heading_offset: HeadingOffset::H2,
}
.into_string();
assert_eq!(output, expect, "original: {}", input);
}

t(
"# Foo bar",
"<h1 id=\"foo-bar\" class=\"section-header\"><a href=\"#foo-bar\">Foo bar</a></h1>",
"<h2 id=\"foo-bar\" class=\"section-header\"><a href=\"#foo-bar\">Foo bar</a></h2>",
);
t(
"## Foo-bar_baz qux",
"<h2 id=\"foo-bar_baz-qux\" class=\"section-header\">\
<a href=\"#foo-bar_baz-qux\">Foo-bar_baz qux</a></h2>",
"<h3 id=\"foo-bar_baz-qux\" class=\"section-header\">\
<a href=\"#foo-bar_baz-qux\">Foo-bar_baz qux</a></h3>",
);
t(
"### **Foo** *bar* baz!?!& -_qux_-%",
"<h3 id=\"foo-bar-baz--qux-\" class=\"section-header\">\
"<h4 id=\"foo-bar-baz--qux-\" class=\"section-header\">\
<a href=\"#foo-bar-baz--qux-\"><strong>Foo</strong> \
<em>bar</em> baz!?!&amp; -<em>qux</em>-%</a>\
</h3>",
</h4>",
);
t(
"#### **Foo?** & \\*bar?!* _`baz`_ ❤ #qux",
"<h4 id=\"foo--bar--baz--qux\" class=\"section-header\">\
"<h5 id=\"foo--bar--baz--qux\" class=\"section-header\">\
<a href=\"#foo--bar--baz--qux\"><strong>Foo?</strong> &amp; *bar?!* \
<em><code>baz</code></em> ❤ #qux</a>\
</h4>",
</h5>",
);
}

#[test]
fn test_header_ids_multiple_blocks() {
let mut map = IdMap::new();
fn t(map: &mut IdMap, input: &str, expect: &str) {
let output =
Markdown(input, &[], map, ErrorCodes::Yes, DEFAULT_EDITION, &None).into_string();
let output = Markdown {
content: input,
links: &[],
ids: map,
error_codes: ErrorCodes::Yes,
edition: DEFAULT_EDITION,
playground: &None,
heading_offset: HeadingOffset::H2,
}
.into_string();
assert_eq!(output, expect, "original: {}", input);
}

t(
&mut map,
"# Example",
"<h1 id=\"example\" class=\"section-header\"><a href=\"#example\">Example</a></h1>",
"<h2 id=\"example\" class=\"section-header\"><a href=\"#example\">Example</a></h2>",
);
t(
&mut map,
"# Panics",
"<h1 id=\"panics\" class=\"section-header\"><a href=\"#panics\">Panics</a></h1>",
"<h2 id=\"panics\" class=\"section-header\"><a href=\"#panics\">Panics</a></h2>",
);
t(
&mut map,
"# Example",
"<h1 id=\"example-1\" class=\"section-header\"><a href=\"#example-1\">Example</a></h1>",
"<h2 id=\"example-1\" class=\"section-header\"><a href=\"#example-1\">Example</a></h2>",
);
t(
&mut map,
"# Main",
"<h1 id=\"main-1\" class=\"section-header\"><a href=\"#main-1\">Main</a></h1>",
"<h2 id=\"main-1\" class=\"section-header\"><a href=\"#main-1\">Main</a></h2>",
);
t(
&mut map,
"# Example",
"<h1 id=\"example-2\" class=\"section-header\"><a href=\"#example-2\">Example</a></h1>",
"<h2 id=\"example-2\" class=\"section-header\"><a href=\"#example-2\">Example</a></h2>",
);
t(
&mut map,
"# Panics",
"<h1 id=\"panics-1\" class=\"section-header\"><a href=\"#panics-1\">Panics</a></h1>",
"<h2 id=\"panics-1\" class=\"section-header\"><a href=\"#panics-1\">Panics</a></h2>",
);
}

Expand Down
Loading