Skip to content

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

Merged
bors merged 3 commits into
rust-lang:masterfrom
Nicholas-Baron:compute_diff_rs
Oct 7, 2021
Merged

Move items related to computing diffs to a separate file#89477
bors merged 3 commits into
rust-lang:masterfrom
Nicholas-Baron:compute_diff_rs

Conversation

@Nicholas-Baron

Copy link
Copy Markdown
Contributor

Work towards #89475.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @Mark-Simulacrum

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

@rust-highfiverust-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

@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.

{
letmut diff_output = File::create(&diff_filename).unwrap();
letmut 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 =
ifletOk(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
Copy Markdown
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
Copy Markdown
Member

@bors r+ rollup

@bors

bors commented Oct 7, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 3760c91 has been approved by Mark-Simulacrum

@borsbors 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:masterOct 7, 2021
@rustbotrustbot 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: 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

@Nicholas-Baron@rust-highfive@Mark-Simulacrum@bors@rustbot