Skip to content

Commit

Permalink
refactor(review-pr): refactor to use hunks instead of Patch + new fil…
Browse files Browse the repository at this point in the history
…e content (#132)

* refactor: use Hunks instead of Patch + new file content

* refactor: move scope comparison to separate module and add tests

* test: add test for fetching hunks for pull request

* chore: fix lint

* docs: fix param type

* fix: trim trailing carriage return

This looks like an platform bug in the upstream parse-diff library.
  • Loading branch information
chingor13 authored Oct 6, 2020
1 parent ffac451 commit cf1baf6
Show file tree
Hide file tree
Showing 20 changed files with 556 additions and 1,568 deletions.
15 changes: 0 additions & 15 deletions src/github-handler/comment-handler/get-hunk-scope-handler/index.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

import {Octokit} from '@octokit/rest';
import {Range, RepoDomain} from '../../../types';
import {RepoDomain, Hunk} from '../../../types';
import {logger} from '../../../logger';
import {parseHunks} from '../../diff-utils';

Expand All @@ -32,7 +32,7 @@ export async function getCurrentPullRequestPatches(
pullNumber: number,
pageSize: number
): Promise<{patches: Map<string, string>; filesMissingPatch: string[]}> {
// TODO support pagination
// TODO: support pagination
const filesMissingPatch: string[] = [];
const files = (
await octokit.pulls.listFiles({
Expand Down Expand Up @@ -78,62 +78,50 @@ index cac8fbc..87f387c 100644
`;

/**
* Given the patch text (for a whole file) for each file,
* get each file's hunk's (part of a file's) range
* @param {Map<string, string>} validPatches patch text from the remote github file
* @returns {Map<string, Range[]>} the range of the remote patch
*/
export function patchTextToRanges(
validPatches: Map<string, string>
): Map<string, Range[]> {
const allValidLineRanges: Map<string, Range[]> = new Map<string, Range[]>();
validPatches.forEach((patch, filename) => {
// get each hunk range in the patch string
try {
const validHunks = parseHunks(_DIFF_HEADER + patch);
// TODO: consider returning hunks instead
const validLineRanges = validHunks.map(hunk => {
return {start: hunk.newStart, end: hunk.newEnd};
});
allValidLineRanges.set(filename, validLineRanges);
} catch (err) {
logger.info(
`Failed to parse the patch of file ${filename}. Resuming parsing patches...`
);
}
});
return allValidLineRanges;
}

/**
* For a pull request, get each remote file's current patch range to identify the scope of each patch as a Map,
* as well as a list of files that cannot have suggestions applied to it within the Pull Request.
* The list of files are a subset of the total out-of-scope files.
* For a pull request, get each remote file's current patch range to identify the scope of each patch as a Map.
* @param {Octokit} octokit the authenticated octokit instance
* @param {RepoDomain} remote the remote repository domain information
* @param {number} pullNumber the pull request number
* @param {number} pageSize the number of files to return per pull request list files query
* @returns {Promise<Object<Map<string, Range[]>, string[]>>} the scope of each file in the pull request and the list of files whose patch data could not be resolved
* @returns {Promise<Map<string, Hunk[]>>} the scope of each file in the pull request
*/
export async function getPullRequestScope(
export async function getPullRequestHunks(
octokit: Octokit,
remote: RepoDomain,
pullNumber: number,
pageSize: number
): Promise<{validFileLines: Map<string, Range[]>; invalidFiles: string[]}> {
try {
const {patches, filesMissingPatch} = await getCurrentPullRequestPatches(
octokit,
remote,
pullNumber,
pageSize
);
const validFileLines = patchTextToRanges(patches);
return {validFileLines, invalidFiles: filesMissingPatch};
} catch (err) {
): Promise<Map<string, Hunk[]>> {
const files = (
await octokit.pulls.listFiles({
owner: remote.owner,
repo: remote.repo,
pull_number: pullNumber,
per_page: pageSize,
})
).data;
const pullRequestHunks: Map<string, Hunk[]> = new Map();
if (files.length === 0) {
logger.error(
'Could not convert the remote pull request file patch text to ranges'
`0 file results have returned from list files query for Pull Request #${pullNumber}. Cannot make suggestions on an empty Pull Request`
);
throw Error('Empty Pull Request');
}
files.forEach(file => {
if (file.patch === undefined) {
// files whose patch is too large do not return the patch text by default
// TODO handle file patches that are too large
logger.warn(
`File ${file.filename} may have a patch that is too large to display patch object.`
);
} else {
const hunks = parseHunks(_DIFF_HEADER + file.patch);
pullRequestHunks.set(file.filename, hunks);
}
});
if (pullRequestHunks.size === 0) {
logger.warn(
'0 patches have been returned. This could be because the patch results were too large to return.'
);
throw err;
}
return pullRequestHunks;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import {Hunk} from '../../../types';

interface PartitionedHunks {
validHunks: Map<string, Hunk[]>;
invalidHunks: Map<string, Hunk[]>;
}

interface PartitionedFileHunks {
validFileHunks: Hunk[];
invalidFileHunks: Hunk[];
}

function partitionFileHunks(
pullRequestHunks: Hunk[],
suggestedHunks: Hunk[]
): PartitionedFileHunks {
// check ranges: the entirety of the old range of the suggested
// hunk must fit inside the new range of the valid Hunks
let i = 0;
let candidateHunk = pullRequestHunks[i];
const validFileHunks: Hunk[] = [];
const invalidFileHunks: Hunk[] = [];
suggestedHunks.forEach(suggestedHunk => {
while (candidateHunk && suggestedHunk.oldStart > candidateHunk.newEnd) {
i++;
candidateHunk = pullRequestHunks[i];
}
if (!candidateHunk) {
return;
}
if (
suggestedHunk.oldStart >= candidateHunk.newStart &&
suggestedHunk.oldEnd <= candidateHunk.newEnd
) {
validFileHunks.push(suggestedHunk);
} else {
invalidFileHunks.push(suggestedHunk);
}
});
return {validFileHunks, invalidFileHunks};
}

/**
* Split suggested hunks into commentable and non-commentable hunks. Compares the new line ranges
* from pullRequestHunks against the old line ranges from allSuggestedHunks.
* @param pullRequestHunks {Map<string, Hunk[]>} The parsed hunks from that represents the valid lines to comment.
* @param allSuggestedHunks {Map<string, Hunk[]>} The hunks that represent suggested changes.
* @returns {PartitionedHunks} split hunks
*/
export function partitionSuggestedHunksByScope(
pullRequestHunks: Map<string, Hunk[]>,
allSuggestedHunks: Map<string, Hunk[]>
): PartitionedHunks {
const validHunks: Map<string, Hunk[]> = new Map();
const invalidHunks: Map<string, Hunk[]> = new Map();
allSuggestedHunks.forEach((suggestedHunks, filename) => {
const pullRequestFileHunks = pullRequestHunks.get(filename);
if (!pullRequestFileHunks) {
// file is not the original PR
invalidHunks.set(filename, suggestedHunks);
return;
}

const {validFileHunks, invalidFileHunks} = partitionFileHunks(
pullRequestFileHunks,
suggestedHunks
);
if (validFileHunks.length > 0) {
validHunks.set(filename, validFileHunks);
}
if (invalidFileHunks.length > 0) {
invalidHunks.set(filename, invalidFileHunks);
}
});
return {validHunks, invalidHunks};
}
26 changes: 17 additions & 9 deletions src/github-handler/comment-handler/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
// limitations under the License.

import {FileDiffContent, RepoDomain} from '../../types';
import {getPullRequestScope} from './get-hunk-scope-handler/';
import {Octokit} from '@octokit/rest';
import {getSuggestionPatches} from './raw-patch-handler';
import {makeInlineSuggestions} from './make-review-handler';
import {logger} from '../../logger';
import {getRawSuggestionHunks} from './raw-patch-handler/raw-hunk-handler';
import {getPullRequestHunks} from './get-hunk-scope-handler/remote-patch-ranges-handler';
import {partitionSuggestedHunksByScope} from './get-hunk-scope-handler/scope-handler';

/**
* Comment on a Pull Request
Expand All @@ -36,21 +37,28 @@ export async function reviewPullRequest(
diffContents: Map<string, FileDiffContent>
): Promise<number | null> {
try {
const {invalidFiles, validFileLines} = await getPullRequestScope(
// get the hunks from the pull request
const pullRequestHunks = await getPullRequestHunks(
octokit,
remote,
pullNumber,
pageSize
);
const {filePatches, outOfScopeSuggestions} = getSuggestionPatches(
diffContents,
invalidFiles,
validFileLines

// get the hunks from the suggested change
const allSuggestedHunks = getRawSuggestionHunks(diffContents);

// split hunks by commentable and uncommentable
const {validHunks, invalidHunks} = partitionSuggestedHunksByScope(
pullRequestHunks,
allSuggestedHunks
);

// create pull request review
const reviewNumber = await makeInlineSuggestions(
octokit,
filePatches,
outOfScopeSuggestions,
validHunks,
invalidHunks,
remote,
pullNumber
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

import {Octokit} from '@octokit/rest';
import {Hunk, Patch, RepoDomain} from '../../../types';
import {Hunk, RepoDomain} from '../../../types';
import {logger} from '../../../logger';
import {buildSummaryComment} from './message-handler';

Expand Down Expand Up @@ -61,25 +61,26 @@ export type PullsCreateReviewParamsComments = {
* @param suggestions
*/
export function buildReviewComments(
suggestions: Map<string, Patch[]>
suggestions: Map<string, Hunk[]>
): Comment[] {
const fileComments: Comment[] = [];
suggestions.forEach((patches: Patch[], fileName: string) => {
patches.forEach(patch => {
if (patch.start === patch.end) {
suggestions.forEach((hunks: Hunk[], fileName: string) => {
hunks.forEach(hunk => {
const newContent = hunk.newContent.join('\n');
if (hunk.newStart === hunk.newEnd) {
const singleComment: SingleLineComment = {
path: fileName,
body: `\`\`\`suggestion\n${patch.newContent}\n\`\`\``,
line: patch.end,
body: `\`\`\`suggestion\n${newContent}\n\`\`\``,
line: hunk.newEnd,
side: 'RIGHT',
};
fileComments.push(singleComment);
} else {
const comment: MultilineComment = {
path: fileName,
body: `\`\`\`suggestion\n${patch.newContent}\n\`\`\``,
start_line: patch.start,
line: patch.end,
body: `\`\`\`suggestion\n${newContent}\n\`\`\``,
start_line: hunk.newStart,
line: hunk.newEnd,
side: 'RIGHT',
start_side: 'RIGHT',
};
Expand All @@ -99,7 +100,7 @@ export function buildReviewComments(
*/
export async function makeInlineSuggestions(
octokit: Octokit,
suggestions: Map<string, Patch[]>,
suggestions: Map<string, Hunk[]>,
outOfScopeSuggestions: Map<string, Hunk[]>,
remote: RepoDomain,
pullNumber: number
Expand Down

This file was deleted.

Loading

0 comments on commit cf1baf6

Please sign in to comment.