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

Move items related to computing diffs to a separate file #89477

Merged
merged 3 commits into from
Oct 7, 2021

Conversation

Nicholas-Baron
Copy link
Contributor

Work towards #89475.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2021
@Mark-Simulacrum
Copy link
Member

There's some diff-related code here as well -

let diff_filename = format!("build/tmp/rustdoc-compare-{}.diff", std::process::id());

I'm not sure how much that is "the same" or can be unified, haven't looked too closely. What do you think?

@Nicholas-Baron
Copy link
Contributor Author

@Mark-Simulacrum While this does seem diff-related, its structure is very different from write_diff and make_diff, suggesting some semantic differences. This makes unification a task I think beyond this PR.

It may be possible to extract some parts as separate functions, as self is not reference heavily in them. However, this may be beyond the scope of this PR as well. The following code block is an example.

{
let mut diff_output = File::create(&diff_filename).unwrap();
let mut wrote_data = false;
for entry in walkdir::WalkDir::new(out_dir) {
let entry = entry.expect("failed to read file");
let extension = entry.path().extension().and_then(|p| p.to_str());
if entry.file_type().is_file()
&& (extension == Some("html".into()) || extension == Some("js".into()))
{
let expected_path =
compare_dir.join(entry.path().strip_prefix(&out_dir).unwrap());
let expected =
if let Ok(s) = std::fs::read(&expected_path) { s } else { continue };
let actual_path = entry.path();
let actual = std::fs::read(&actual_path).unwrap();
let diff = unified_diff::diff(
&expected,
&expected_path.to_string_lossy(),
&actual,
&actual_path.to_string_lossy(),
3,
);
wrote_data |= !diff.is_empty();
diff_output.write_all(&diff).unwrap();
}
}

@Mark-Simulacrum
Copy link
Member

Hm. Ok. Well, I guess we can add a "why do we have two different diff implementations" (unified-diff and diff crates) to the list of open questions...

But regardless I think much of the code can be moved into this module, if not unified just yet -- it looks pretty separate from being rustdoc specific to me, beyond the minor filter on extensions (which seems easy to pass in).

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 7, 2021

📌 Commit 3760c91 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 7, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#89298 (Issue 89193 - Fix ICE when using `usize` and `isize` with SIMD gathers )
 - rust-lang#89461 (Add `deref_into_dyn_supertrait` lint.)
 - rust-lang#89477 (Move items related to computing diffs to a separate file)
 - rust-lang#89559 (RustWrapper: adapt for LLVM API change)
 - rust-lang#89585 (Emit item no type error even if type inference fails)
 - rust-lang#89596 (Make cfg imply doc(cfg))
 - rust-lang#89615 (Add InferCtxt::with_opaque_type_inference to get_body_with_borrowck_facts)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 48548c9 into rust-lang:master Oct 7, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 7, 2021
@Nicholas-Baron Nicholas-Baron deleted the compute_diff_rs branch October 7, 2021 21:04
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.

5 participants