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

Only fetch PR diff once. #1749

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Only fetch PR diff once. #1749

merged 1 commit into from
Nov 17, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 17, 2023

This adds an optimization and simplification for getting the diff of a PR. Previously, triagebot would fetch the /compare endpoint three times for every PR (for auto-assignment, auto-labels, and mentions). This changes it so that it only fetches it once, and caches the result in the PullRequestDetails.

This also changes it so that the diff gets parsed early on, instead of having every caller parse it. This should help make it a little more structured. Thus, diff() returns a Vec<FileDiff>, where each element is a single file.

A walkthrough to help reviewing this PR:

  1. Added a FileDiff type to represent a diff to a single file.
  2. PullRequestDetails adds a OnceCell to cache the PR diff.
  3. Issue.diff() now returns a parsed result (instead of just a string), and caches the result.
  4. files_changed was changed to parse_diff to return a structured format.
  5. All the callers of Issue.diff() were updated to use the new format.
  6. The assign.rs input handler was changed to fetch the diff in handle_input instead of parse_input, due to not being able to pass a reference (and it wasn't important which function it was called in).
  7. Auto-assignment was doing its own diff parsing which is no longer necessary.

@ehuss ehuss merged commit 0a68321 into rust-lang:master Nov 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants