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 1 commit
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
6 changes: 3 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 @@ -46,7 +46,7 @@ impl ExternalHtml {
error_codes: codes,
edition,
playground,
heading_level: 1
heading_offset: HeadingOffset::H2,
GuillaumeGomez marked this conversation as resolved.
Show resolved Hide resolved
}
.into_string()
);
Expand All @@ -62,7 +62,7 @@ impl ExternalHtml {
error_codes: codes,
edition,
playground,
heading_level: 1
heading_offset: HeadingOffset::H2,
}
.into_string()
);
Expand Down
41 changes: 28 additions & 13 deletions src/librustdoc/html/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
//! 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();
Expand All @@ -19,7 +19,7 @@
//! error_codes: ErrorCodes::Yes,
//! edition: Edition::Edition2015,
//! playground: &None,
//! heading_level: 1
//! heading_offset: HeadingOffset::H2,
//! };
//! let html = md.into_string();
//! // ... something using html
Expand Down Expand Up @@ -75,6 +75,16 @@ 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> {
Expand All @@ -89,8 +99,8 @@ pub struct Markdown<'a> {
pub edition: Edition,
pub playground: &'a Option<Playground>,
/// Offset at which we render headings.
/// E.g. if `heading_level: 1`, then `# something` renders an `<h2>` instead of `<h1>`
pub heading_level: u32,
/// 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>(
Expand Down Expand Up @@ -502,12 +512,17 @@ struct HeadingLinks<'a, 'b, 'ids, I> {
toc: Option<&'b mut TocBuilder>,
buf: VecDeque<SpannedEvent<'a>>,
id_map: &'ids mut IdMap,
level: u32,
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, level: u32) -> Self {
HeadingLinks { inner: iter, toc, buf: VecDeque::new(), id_map: ids, level }
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 @@ -544,7 +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.level, MAX_HEADER_LEVEL);
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 @@ -1027,7 +1042,7 @@ impl Markdown<'_> {
error_codes: codes,
edition,
playground,
heading_level,
heading_offset,
} = self;

// This is actually common enough to special-case
Expand All @@ -1049,7 +1064,7 @@ impl Markdown<'_> {

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

let p = HeadingLinks::new(p, None, &mut ids, heading_level);
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 @@ -1071,7 +1086,7 @@ impl MarkdownWithToc<'_> {
let mut toc = TocBuilder::new();

{
let p = HeadingLinks::new(p, Some(&mut toc), &mut ids, 0);
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 @@ -1100,7 +1115,7 @@ impl MarkdownHtml<'_> {

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

let p = HeadingLinks::new(p, None, &mut ids, 0);
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 @@ -1318,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, 0));
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
6 changes: 3 additions & 3 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 @@ -154,7 +154,7 @@ fn test_header() {
error_codes: ErrorCodes::Yes,
edition: DEFAULT_EDITION,
playground: &None,
heading_level: 1,
heading_offset: HeadingOffset::H2,
}
.into_string();
assert_eq!(output, expect, "original: {}", input);
Expand Down Expand Up @@ -196,7 +196,7 @@ fn test_header_ids_multiple_blocks() {
error_codes: ErrorCodes::Yes,
edition: DEFAULT_EDITION,
playground: &None,
heading_level: 1,
heading_offset: HeadingOffset::H2,
}
.into_string();
assert_eq!(output, expect, "original: {}", input);
Expand Down
42 changes: 26 additions & 16 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ use crate::html::format::{
href, print_abi_with_space, print_constness_with_space, print_default_space,
print_generic_bounds, print_where_clause, Buffer, HrefError, PrintWithSpace,
};
use crate::html::markdown::{Markdown, MarkdownHtml, MarkdownSummaryLine};
use crate::html::markdown::{HeadingOffset, Markdown, MarkdownHtml, MarkdownSummaryLine};

/// A pair of name and its optional document.
crate type NameDoc = (String, Option<String>);
Expand Down Expand Up @@ -475,16 +475,16 @@ fn document(
cx: &Context<'_>,
item: &clean::Item,
parent: Option<&clean::Item>,
level: u32,
heading_offset: HeadingOffset,
) {
if let Some(ref name) = item.name {
info!("Documenting {}", name);
}
document_item_info(w, cx, item, parent);
if parent.is_none() {
document_full_collapsible(w, item, cx, level);
document_full_collapsible(w, item, cx, heading_offset);
} else {
document_full(w, item, cx, level);
document_full(w, item, cx, heading_offset);
}
}

Expand All @@ -494,7 +494,7 @@ fn render_markdown(
cx: &Context<'_>,
md_text: &str,
links: Vec<RenderedLink>,
heading_level: u32,
heading_offset: HeadingOffset,
) {
let mut ids = cx.id_map.borrow_mut();
write!(
Expand All @@ -507,7 +507,7 @@ fn render_markdown(
error_codes: cx.shared.codes,
edition: cx.shared.edition(),
playground: &cx.shared.playground,
heading_level,
heading_offset,
}
.into_string()
)
Expand Down Expand Up @@ -544,20 +544,30 @@ fn document_short(
}
}

fn document_full_collapsible(w: &mut Buffer, item: &clean::Item, cx: &Context<'_>, level: u32) {
document_full_inner(w, item, cx, true, level);
fn document_full_collapsible(
w: &mut Buffer,
item: &clean::Item,
cx: &Context<'_>,
heading_offset: HeadingOffset,
) {
document_full_inner(w, item, cx, true, heading_offset);
}

fn document_full(w: &mut Buffer, item: &clean::Item, cx: &Context<'_>, level: u32) {
document_full_inner(w, item, cx, false, level);
fn document_full(
w: &mut Buffer,
item: &clean::Item,
cx: &Context<'_>,
heading_offset: HeadingOffset,
) {
document_full_inner(w, item, cx, false, heading_offset);
}

fn document_full_inner(
w: &mut Buffer,
item: &clean::Item,
cx: &Context<'_>,
is_collapsible: bool,
level: u32,
heading_offset: HeadingOffset,
) {
if let Some(s) = cx.shared.maybe_collapsed_doc_value(item) {
debug!("Doc block: =====\n{}\n=====", s);
Expand All @@ -568,10 +578,10 @@ fn document_full_inner(
<span>Expand description</span>\
</summary>",
);
render_markdown(w, cx, &s, item.links(cx), level);
render_markdown(w, cx, &s, item.links(cx), heading_offset);
w.write_str("</details>");
} else {
render_markdown(w, cx, &s, item.links(cx), level);
render_markdown(w, cx, &s, item.links(cx), heading_offset);
}
}
}
Expand Down Expand Up @@ -1340,7 +1350,7 @@ fn render_impl(
// because impls can't have a stability.
if item.doc_value().is_some() {
document_item_info(&mut info_buffer, cx, it, Some(parent));
document_full(&mut doc_buffer, item, cx, 4);
document_full(&mut doc_buffer, item, cx, HeadingOffset::H5);
short_documented = false;
} else {
// In case the item isn't documented,
Expand All @@ -1358,7 +1368,7 @@ fn render_impl(
} else {
document_item_info(&mut info_buffer, cx, item, Some(parent));
if rendering_params.show_def_docs {
document_full(&mut doc_buffer, item, cx, 4);
document_full(&mut doc_buffer, item, cx, HeadingOffset::H5);
short_documented = false;
}
}
Expand Down Expand Up @@ -1599,7 +1609,7 @@ fn render_impl(
error_codes: cx.shared.codes,
edition: cx.shared.edition(),
playground: &cx.shared.playground,
heading_level: 1
heading_offset: HeadingOffset::H2
}
.into_string()
);
Expand Down
Loading