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

refactor(review-pr): refactor to use hunks instead of Patch + new file content #132

Merged
merged 7 commits into from
Oct 6, 2020

Conversation

chingor13
Copy link
Contributor

@chingor13 chingor13 commented Oct 5, 2020

This is a rather large refactor:

  • remove Patch, Range types in favor of using Hunk everywhere
  • add newContent field to Hunk
    • this will enable us to provide a raw diff as an input for review comments as we only need the new suggested content, not the entire file
  • simplifies the main reviewPullRequest logic. we do not need to track invalid files separately (just don't include them in the valid suggestion lines)
    • fetch valid lines from PR
    • fetch valid lines from suggestion
    • partition into valid/invalid suggestions
    • create pull request review with comments

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 5, 2020
@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #132 into master will decrease coverage by 1.83%.
The diff coverage is 92.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   88.17%   86.34%   -1.84%     
==========================================
  Files          25       22       -3     
  Lines        2384     2145     -239     
  Branches      180      150      -30     
==========================================
- Hits         2102     1852     -250     
- Misses        281      292      +11     
  Partials        1        1              
Impacted Files Coverage Δ
...-hunk-scope-handler/remote-patch-ranges-handler.ts 90.55% <72.22%> (-6.58%) ⬇️
...nt-handler/get-hunk-scope-handler/scope-handler.ts 96.66% <96.66%> (ø)
src/github-handler/comment-handler/index.ts 100.00% <100.00%> (ø)
...ler/make-review-handler/upload-comments-handler.ts 99.31% <100.00%> (+<0.01%) ⬆️
src/github-handler/diff-utils.ts 100.00% <100.00%> (ø)
src/types/index.ts 98.24% <100.00%> (-0.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffac451...764e458. Read the comment docs.

@chingor13 chingor13 changed the title refactor: refactor to use hunks instead of Patch + new file content refactor(review-pr): refactor to use hunks instead of Patch + new file content Oct 5, 2020
@chingor13 chingor13 marked this pull request as ready for review October 5, 2020 23:39
@chingor13 chingor13 requested a review from a team October 5, 2020 23:39
@chingor13 chingor13 requested a review from a team as a code owner October 5, 2020 23:39
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This looks really good to me, great to see us delete 1000 lines.

return {validFileLines, invalidFiles: filesMissingPatch};
} catch (err) {
): Promise<Map<string, Hunk[]>> {
const files = (
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use octokit's iterator API, you can do something along the lines of:

for await () {}

And avoid pagination concerns if there are more than 100 files.

`File ${file.filename} may have a patch that is too large to display patch object.`
);
} else {
const hunks = parseHunks(_DIFF_HEADER + file.patch);
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like this new logic is biting you, with regards to windows line endings.

In test you might want to add a helper that just standardizes on \n.

sha: 'a1d470fa4d7b04450715e3e02d240a34517cd988',
filename: 'Readme.md',
status: 'modified',
additions: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any value in pulling this into a fixture? maybe not worth it if we're only going to have one smoke test, but if we're going to add a few payloads like this over time, perhaps we could collect them in a folder.

This looks like an platform bug in the upstream parse-diff library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants