-
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
Improve the Pulldown/hoedown warnings #44238
Conversation
Also checks for differences after eliminating whitespace-only diffs. Renames get_html_diff
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.
Couple questions
if !markdown_warnings.is_empty() { | ||
println!("WARNING: documentation for this crate may be rendered \ | ||
differently using the new Pulldown renderer."); | ||
println!(" See https://github.com/rust-lang/rust/issues/44229 for details."); |
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.
Do we want all these println!
s to be on stdout or stderr?
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 wanted them to be on stderr, but using eprintln
gave no output - I'm not sure if Rustbuild or something else is swallowing stderr. I can try again to confirm I wasn't missing something, but I fear we may be stuck with stdout.
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.
If that's the case i won't object. I'm not sure if rustdoc has printed anything that didn't come from a panic before now.
|
||
// A short, single-line view of `s`. | ||
fn concise_str(s: &str) -> String { | ||
if s.contains('\n') { |
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.
From the rest of the diff, i'm assuming that the text fed to this function has always been run through one of the renderers. Can either of them use Windows line endings? Does it matter?
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.
Good catch, I can adjust that.
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.
Just using something like .lines().next()
would catch it, i assume.
From conversation on IRC, it seems like stderr is getting swallowed somewhere along the line. Possibly worth investigating, but not now. r=me pending travis. |
scx: &SharedContext) | ||
-> fmt::Result { | ||
let hoedown_output = format!("{}", Markdown(md_text, RenderType::Hoedown)); | ||
// We only emit warnings if the user has opted-in to Pulldown rendering. |
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 understand: you said you wanted warnings all the time and it was one of the reasons to delay the merge. Why this change?
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 core team thought that we should have this off by default until we've investigated (and probably remedied) the false positives and also communicated that these warnings are going to happen, so as not to surprise users. Sorry, for the back and forth here - my bad for not checking with the core team before asking to turn on by default.
write!(w, "<div class='docblock'>{}{}</div>", prefix, output) | ||
} | ||
|
||
// Returns true iff s1 and s2 match, ignoring whitespace. |
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.
"iff"
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.
It's an abbreviation for 'if and only if'
This is just undoing changes from rust-lang#41991 because we are not running markdown rendering twice.
@bors: r=@QuietMisdreavus p=10 |
📌 Commit 1d6d09f has been approved by |
⌛ Testing commit 1d6d09f with merge 54793738b28b1495d20de370b03381e0a0e3ceb4... |
💔 Test failed - status-travis |
|
// A short, single-line view of `s`. | ||
fn concise_str(s: &str) -> String { | ||
if s.contains('\n') { | ||
return format!("{}...", s.lines().next().expect("Impossible! We just found a newline")); |
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.
If the first line is really long then this should probably only print the start of it like the case below.
return format!("{}...", s.lines().next().expect("Impossible! We just found a newline")); | ||
} | ||
if s.len() > 70 { | ||
return format!("{} ... {}", &s[..50], &s[s.len()-20..]); |
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.
This will panic if 50
or s.len()-20
isn't on a character boundary.
_ => true, | ||
} | ||
}) | ||
.collect::<Vec<_>>(); |
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.
Vec::retain
would be cleaner.
assert!(match_non_whitespace("abc xyz", "abc xyz")); | ||
assert!(match_non_whitespace("abc xyz", "abc\nxyz")); | ||
assert!(match_non_whitespace("abc xyz", "abcxyz")); | ||
assert!(match_non_whitespace("abcxyz", "abc xyz")); |
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.
Why would these be considered equal? match_non_whitespace
is almost identical to s1.split_whitespace().eq(s2.split_whitespace())
except for the two cases above. However, the whitepace should be handled in html_diff
anyway as whether it matters depends on the context. For example whitespace is very important inside <pre>
tags.
Improve the Pulldown/hoedown warnings cc #44229 r? @QuietMisdreavus
☀️ Test successful - status-appveyor, status-travis |
Fixup some nits from rust-lang#44238 r? @ollie27
Fixup some nits from rust-lang#44238 r? @ollie27
Fixup some nits from rust-lang#44238 r? @ollie27
cc #44229
r? @QuietMisdreavus