-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Support lists and stylings in more places for rustc --explain
#126994
Conversation
rustbot has assigned @compiler-errors. Use |
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.
Thanks! Couple small suggestions
/// Find first line that isn't empty or doesn't start with whitespace, that will | ||
/// be our contents |
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.
Did you mean to update rather than remove?
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.
Meant to remove it since the function name is clear, the previous comment confused me a bit
_ if let Some(strong @ (b"**" | b"__")) = loop_buf.get(..2) => { | ||
parse_simple_pat(loop_buf, strong, strong, Po::None, MdTree::Strong) | ||
} | ||
(_, Newline | Whitespace) if loop_buf.starts_with(STG) => { | ||
parse_simple_pat(loop_buf, STG, STG, Po::None, MdTree::Strong) | ||
_ if let Some(emph @ (b"*" | b"_")) = loop_buf.get(..1) => { | ||
parse_simple_pat(loop_buf, emph, emph, Po::None, MdTree::Emphasis) | ||
} | ||
(_, Newline | Whitespace) if loop_buf.starts_with(STK) => { | ||
_ if loop_buf.starts_with(STK) => { | ||
parse_simple_pat(loop_buf, STK, STK, Po::None, MdTree::Strikethrough) | ||
} | ||
(_, Newline | Whitespace) if loop_buf.starts_with(ANC_S) => { | ||
_ if loop_buf.starts_with(ANC_S) => { |
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.
The comment, code, and *
/**
/~~
changes look correct, but _
/__
and <...>
anchors need the preceding whitespace so snake case doesn't turn into italics (gh agrees with commonmark here):
foo*bar*
foobarfoo**bar**
foobarfoo_bar_
foo_bar_foo__bar__
foo__bar__foo~~bar~
foobarfoo`bar`
foobar
foo<www.docs.rs>
foo<www.docs.rs>
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.
My impression was it's going for concise over correct hence the hand rolled markdown parser, there's a bunch of other stuff like (_foo_)
should italicise but not _foo _bar
or foo* bar*
. As far as I can tell these do not occur in the error code docs though, so the question is how much do we want to support?
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.
Yeah, it was intended to be pretty simple and just hopefully never receive bad markdown 😆.
Adding Boundary
to Prev
and then setting it with something like x.is_ascii_punctuation() && ~[b'_', b'*', b'~].contains(&x)
might be slightly more accurate to match Newline | Whitespace | Boundary
. No need for you to do that here though, I'll probably play around with it a bit in a followup.
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.
A punctuation boundary sounds good, should I still make the #126994 (comment) change or leave it to you when you implement that?
If it ever will support nested formatting then just x.is_ascii_punctuation()
is closer I think, since ___foo__ bar_
is valid as foo bar
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.
Up to you, I'll follow up if you prefer not to make the chance here :)
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.
Made the change since you would've had to unmerge the *
/_
arms either way
There's also a stdout test in markdown/tests/input.md if you want to verify the result (should work with --bless) |
1526233
to
5824ab1
Compare
@rustbot ready |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#125886 (Migrate run make issue 15460) - rust-lang#126898 (Migrate `run-make/link-framework` to `rmake.rs`) - rust-lang#126994 (Support lists and stylings in more places for `rustc --explain`) - rust-lang#127990 (Migrate `lto-linkage-used-attr`, `no-duplicate-libs` and `pgo-gen-no-imp-symbols` `run-make` tests to rmake) - rust-lang#128060 (Fix inclusion of `wasm-component-ld` in dist artifacts) - rust-lang#128082 (Note closure captures when reporting cast to fn ptr failed) - rust-lang#128098 (make it possible to disable download-rustc if it's incompatible) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#126994 - Alexendoo:explain-markdown, r=tgross35 Support lists and stylings in more places for `rustc --explain` Adds support for `*foo*`, stylings not immediately following whitespace e.g. ``(`Foo`)`` and lists starting with whitespace: ```md * previously supported ``` ```md * now also supported ``` These are fairly common in the existing error docs, some before/after examples: ### E0460 ![image](https://github.com/rust-lang/rust/assets/1830331/4d0dc5dd-b71f-48b1-97ae-9f7199e952ed) ![image](https://github.com/rust-lang/rust/assets/1830331/4bbcb1e4-99ba-4d0d-b338-fe19d96a5eb1) ### E0059 ![image](https://github.com/rust-lang/rust/assets/1830331/8457f69a-3126-4777-aa4a-953f7b29f59b) ![image](https://github.com/rust-lang/rust/assets/1830331/ac2189f8-512e-4b3b-886d-6c4a619d17f2)
Adds support for
*foo*
, stylings not immediately following whitespace e.g.(`Foo`)
and lists starting with whitespace:* previously supported
* now also supported
These are fairly common in the existing error docs, some before/after examples:
E0460
E0059