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

Reduce false positives number in rustdoc html diff #44347

Merged
merged 1 commit into from
Sep 10, 2017

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Sep 5, 2017

cc @rust-lang/dev-tools
r? @nrc

Very simple trick but should lighten html diff a bit

elem.path, s1, s2);
if elem_text.split("\n")
.zip(opposite_elem_text.split("\n"))
.find(|&(a, b)| a.trim() != b.trim()).is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

small note: .find(...).is_some() is the same as .any(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good point!

@ollie27
Copy link
Member

ollie27 commented Sep 5, 2017

What false positives does this avoid that

let differences = differences.into_iter()
.filter(|s| {
match *s {
html_diff::Difference::NodeText { ref elem_text,
ref opposite_elem_text,
.. }
if match_non_whitespace(elem_text, opposite_elem_text) => false,
_ => true,
}
})
.collect::<Vec<_>>();
(which is cleaned up a bit in #44329) won't?

EDIT: actually I noticed that this just reduces the output a bit for already found differences, so it would be good to see some before and afters.

@GuillaumeGomez
Copy link
Member Author

@ollie27: Yes, it works on already found differences (like almost all upcoming rustdoc PRs will most likely).

@QuietMisdreavus
Copy link
Member

[00:31:59] error[E0308]: mismatched types
[00:31:59] --> /checkout/src/librustdoc/html/render.rs:665:31
[00:31:59] |
[00:31:59] 665 | .any(|&(a, b)| a.trim() != b.trim()) {
[00:31:59] | ^^^^^^^ expected tuple, found reference
[00:31:59] |
[00:31:59] = note: expected type (&str, &str)
[00:31:59] found type &_

oh hey, .any() gives the real item, not a reference

@GuillaumeGomez
Copy link
Member Author

Damn!

@QuietMisdreavus
Copy link
Member

I'm not entirely sure which specific entries this will help, but if you say it helps, i'm willing to land this anyway. However, since #44350 changed the signature and layout of render_difference, this will need to be rebased on top of that one when it's landed. r=me once all that happens.

@GuillaumeGomez
Copy link
Member Author

I think we should merge this one first if you don't mind. The second one will be delayed.

@QuietMisdreavus
Copy link
Member

Yeah, since that other one got delayed, i'll go ahead and push this one forward.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 6, 2017

📌 Commit 502e707 has been approved by QuietMisdreavus

@alexcrichton alexcrichton added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 6, 2017
@frewsxcv
Copy link
Member

frewsxcv commented Sep 7, 2017

@bors rollup

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 8, 2017
…ve, r=QuietMisdreavus

Reduce false positives number in rustdoc html diff

cc @rust-lang/dev-tools
r? @nrc

Very simple trick but should lighten html diff a bit
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 9, 2017
…ve, r=QuietMisdreavus

Reduce false positives number in rustdoc html diff

cc @rust-lang/dev-tools
r? @nrc

Very simple trick but should lighten html diff a bit
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 9, 2017
…ve, r=QuietMisdreavus

Reduce false positives number in rustdoc html diff

cc @rust-lang/dev-tools
r? @nrc

Very simple trick but should lighten html diff a bit
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 10, 2017
…ve, r=QuietMisdreavus

Reduce false positives number in rustdoc html diff

cc @rust-lang/dev-tools
r? @nrc

Very simple trick but should lighten html diff a bit
bors added a commit that referenced this pull request Sep 10, 2017
Rollup of 13 pull requests

- Successful merges: #44262, #44329, #44332, #44347, #44372, #44384, #44387, #44396, #44449, #44451, #44457, #44464, #44467
- Failed merges:
@bors bors merged commit 502e707 into rust-lang:master Sep 10, 2017
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-false-positive branch September 10, 2017 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants