Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(framework-core): support inline multiline comments suggestions o…
…n pull requests (#105) * feat(patch text to hunk bounds): support regex for patch texts (#83) * fix(patch text to hunk bounds): support regex for patch texts * more comments and more tests * fix(framework-core): core-library get remote patch ranges (#84) * fix(framework-core): given files old content and new content, compute the valid hunks (#86) * fix(framework-core): parse raw changes to ranges * refactor(framework-core): rename modules, functions, & re-org project structure (#89) * fix(framework-core): hunk to patch object (#91) * feat: build failure message from invalid hunks (#90) * test: add failing stub and test for building the failure message * fix: implement message building * fix: use original line numbers in error message * docs: add docstring * docs: add note about empty input returning empty string * feat(framework-core): comment on prs given suggestions (#93) * feat(framework-core): main interface for create review on a pull request (#114) * feat(framework-core): main interface for create review on a pull request * docs: fix typo * nits and typos... * gts lint warning fix * fix(framework-core): combine review comments (#116) * fix(framework-core): collapsing timeline and inline comments into single review * test: fixed imports * added case when there are out of scope suggestions and no valid suggestions * feat(framework-core): return review number and variable renaming (#117) * feat(framework-core): return review number and variable renaming * lint Co-authored-by: Jeff Ching <chingor@google.com> Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com> Co-authored-by: Benjamin E. Coe <bencoe@google.com>
- Loading branch information
4 people
committed
Sep 25, 2020
1 parent
c61da3e
commit 415fb8a
Showing
29 changed files
with
3,887 additions
and
8 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
124 changes: 124 additions & 0 deletions
124
src/github-handler/comment-handler/get-hunk-scope-handler/github-patch-text-handler.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| // 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 {PatchSyntaxError, Range} from '../../../types'; | ||
| import {logger} from '../../../logger'; | ||
|
|
||
| const REGEX_INDEX_OF_UPDATED_HUNK = 2; | ||
|
|
||
| /** | ||
| * @@ -<start line original>,<offset> +<start line updated>,<offset> @@ | ||
| * i.e. @@ -132,7 +132,7 @@ | ||
| */ | ||
| const REGEX_MULTILINE_RANGE = /@@ -([0-9]+,[0-9]+) \+([0-9]+,[0-9]+) @@/g; | ||
|
|
||
| /** | ||
| * @@ -<start line original> +<start line updated>,<offset> @@ | ||
| * i.e. a deletion @@ -1 +0,0 @@ | ||
| */ | ||
| const REGEX_ONELINE_TO_MULTILINE_RANGE = /@@ -([0-9]+) \+([0-9]+,[0-9]+) @@/g; | ||
| /** | ||
| * @@ -<line original> +<line updated> @@ | ||
| * i.e. @@ -1 +1 @@ | ||
| */ | ||
| const REGEX_ONELINE_RANGE = /@@ -([0-9]+) \+([0-9]+) @@/g; | ||
| /** | ||
| * @@ -<start line original>,<offset> +<line updated> @@ | ||
| * i.e. file creation @@ -0,0 +1 @@ | ||
| */ | ||
| const REGEX_MULTILINE_TO_ONELINE_RANGE = /@@ -([0-9]+,[0-9]+) \+([0-9]+) @@/g; | ||
|
|
||
| /** | ||
| * Parses the GitHub line-based patch text. | ||
| * Throws an error if the patch text is undefined, null, or not a patch text. | ||
| * Example output of one output of one regex exec: | ||
| * | ||
| * '@@ -0,0 +1,12 @@\n', // original text | ||
| * '0,0', // original hunk | ||
| * '1,12', // new hunk | ||
| * index: 0, | ||
| * input: '@@ -0,0 +1,12 @@\n+Hello world%0A', | ||
| * groups: undefined | ||
| * @param {string} patchText | ||
| * @returns patch ranges | ||
| */ | ||
| export function getGitHubPatchRanges(patchText: string): Range[] { | ||
| if (typeof patchText !== 'string') { | ||
| throw new TypeError('GitHub patch text must be a string'); | ||
| } | ||
| const ranges: Range[] = []; | ||
| // CASE I: multiline patch ranges | ||
| // includes non-first single-line patches | ||
| // i.e. @@ -3,4 +3,4 @@ | ||
| // which only edits line 3, but is still a multiline patch range | ||
| for ( | ||
| let patch = REGEX_MULTILINE_RANGE.exec(patchText); | ||
| patch !== null; | ||
| patch = REGEX_MULTILINE_RANGE.exec(patchText) | ||
| ) { | ||
| // stricly interested in the updated/current github file content | ||
| const patchData = patch[REGEX_INDEX_OF_UPDATED_HUNK].split(','); | ||
| // the line number ranges of the updated text | ||
| const start = parseInt(patchData[0]); | ||
| const offset = parseInt(patchData[1]); | ||
| const range: Range = {start, end: start + offset}; | ||
| ranges.push(range); | ||
| } | ||
| // CASE II: oneline text becomes multiline text | ||
| for ( | ||
| let patch = REGEX_ONELINE_TO_MULTILINE_RANGE.exec(patchText); | ||
| patch !== null; | ||
| patch = REGEX_ONELINE_TO_MULTILINE_RANGE.exec(patchText) | ||
| ) { | ||
| // stricly interested in the updated/current github file content | ||
| const patchData = patch[REGEX_INDEX_OF_UPDATED_HUNK].split(','); | ||
| // the line number ranges of the updated text | ||
| const start = parseInt(patchData[0]); | ||
| const offset = parseInt(patchData[1]); | ||
| const range: Range = {start, end: start + offset}; | ||
| ranges.push(range); | ||
| } | ||
| // CASE III: first line of text updated | ||
| for ( | ||
| let patch = REGEX_ONELINE_RANGE.exec(patchText); | ||
| patch !== null; | ||
| patch = REGEX_ONELINE_RANGE.exec(patchText) | ||
| ) { | ||
| // stricly interested in the updated/current github file content | ||
| // the line number ranges of the updated text | ||
| const start = parseInt(patch[REGEX_INDEX_OF_UPDATED_HUNK]); | ||
| const range: Range = {start, end: start + 1}; | ||
| ranges.push(range); | ||
| } | ||
| // CASE IV: Multiline range is reduced to one line | ||
| // 0,0 constitutes a multi-line range | ||
| for ( | ||
| let patch = REGEX_MULTILINE_TO_ONELINE_RANGE.exec(patchText); | ||
| patch !== null; | ||
| patch = REGEX_MULTILINE_TO_ONELINE_RANGE.exec(patchText) | ||
| ) { | ||
| // stricly interested in the updated/current github file content | ||
| // the line number ranges of the updated text | ||
| const start = parseInt(patch[REGEX_INDEX_OF_UPDATED_HUNK]); | ||
| const range: Range = {start, end: start + 1}; | ||
| ranges.push(range); | ||
| } | ||
| if (!ranges.length) { | ||
| logger.error( | ||
| `Unexpected input patch text provided. Expected "${patchText}" to be of format @@ -<number>[,<number>] +<number>[,<number>] @@` | ||
| ); | ||
| throw new PatchSyntaxError('Unexpected patch text format'); | ||
| } | ||
| return ranges; | ||
| } |
16 changes: 16 additions & 0 deletions
16
src/github-handler/comment-handler/get-hunk-scope-handler/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // 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. | ||
|
|
||
| export {getPullRequestScope} from './remote-patch-ranges-handler'; | ||
| export {getGitHubPatchRanges} from './github-patch-text-handler'; |
127 changes: 127 additions & 0 deletions
127
src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| // 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 {Octokit} from '@octokit/rest'; | ||
| import {Range, RepoDomain} from '../../../types'; | ||
| import {getGitHubPatchRanges} from './github-patch-text-handler'; | ||
| import {logger} from '../../../logger'; | ||
|
|
||
| /** | ||
| * For a pull request, get each remote file's patch text asynchronously | ||
| * Also get the list of files whose patch data could not be returned | ||
| * @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 results to return per page | ||
| * @returns {Promise<Object<PatchText, string[]>>} the stringified patch data for each file and the list of files whose patch data could not be resolved | ||
| */ | ||
| export async function getCurrentPullRequestPatches( | ||
| octokit: Octokit, | ||
| remote: RepoDomain, | ||
| pullNumber: number, | ||
| pageSize: number | ||
| ): Promise<{patches: Map<string, string>; filesMissingPatch: string[]}> { | ||
| // TODO support pagination | ||
| const filesMissingPatch: string[] = []; | ||
| const files = ( | ||
| await octokit.pulls.listFiles({ | ||
| owner: remote.owner, | ||
| repo: remote.repo, | ||
| pull_number: pullNumber, | ||
| per_page: pageSize, | ||
| }) | ||
| ).data; | ||
| const patches: Map<string, string> = new Map<string, string>(); | ||
| if (files.length === 0) { | ||
| logger.error( | ||
| `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.` | ||
| ); | ||
| filesMissingPatch.push(file.filename); | ||
| } else { | ||
| patches.set(file.filename, file.patch); | ||
| } | ||
| }); | ||
| if (patches.size === 0) { | ||
| logger.warn( | ||
| '0 patches have been returned. This could be because the patch results were too large to return.' | ||
| ); | ||
| } | ||
| return {patches, filesMissingPatch}; | ||
| } | ||
|
|
||
| /** | ||
| * 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 validLineRanges = getGitHubPatchRanges(patch); | ||
| 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. | ||
| * @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 | ||
| */ | ||
| export async function getPullRequestScope( | ||
| 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) { | ||
| logger.error( | ||
| 'Could not convert the remote pull request file patch text to ranges' | ||
| ); | ||
| throw err; | ||
| } | ||
| } |
Oops, something went wrong.