feat(framework-core): support inline multiline comments suggestions on pull requests

README.md

@@ -51,6 +51,56 @@ async function main() {
 }
 
 ```
+### reviewPullRequest(options)
+
+The `reviewPullRequest()` method creates a code suggestion review on a GitHub Pull request with the files given as input.
+From these files, calculate the hunk diff and make all the multiline code suggestion review comments on a given pull request with these hunks given
+that they are in scope of the pull request. Outof scope suggestions are not made.
+
+In-scope suggestions are specifically: suggestions whose file is in-scope of the pull request,
+and suggestions whose diff hunk is a subset of the pull request's files hunks.
+
+ If a file is too large to load in the review, it is skipped in the suggestion phase.
+
+ If the program terminates without exception, a timeline comment will be made with all errors or suggestions that could not be made.
+
+#### Syntax
+`reviewPullRequest(octokit, rawChanges, config [, logger])`
+
+#### Parameters
+#### `octokit`
+*octokit* <br>
+**Required.** An authenticated [octokit](https://github.com/octokit/rest.js/) instance.
+
+#### `rawChanges`
+*Map<string, RawContent> | null | undefined* <br>
+**Required.** A set of files with their respective original raw file content and the new file content. If it is null, the empty map, or undefined, a review is not made.
+
+**RawContent Object**
+|      field      |     type  	|   description	|
+|---------------	|-----------	|-------------	|
+|   oldContent	|   `string`	| **Required.** The older version of a file.  |
+|   newContent	|   `string`	| **Required.** The newer version of a file. |
+
+#### `options`
+*Review Pull Request Options Object* <br>
+**Required.**
+
+**Review Pull Request Options Object**
+|      field      |     type  	|   description	|
+|---------------	|-----------	|-------------	|
+|   repo	|   `string`	| **Required.** The repository containing the pull request.  |
+|   owner	|   `string`	| **Required.** The owner of the repository. |
+|   pullNumber	  |   `number`	| **Required.** The GitHub Pull Request number.  |
+|   pageSize	  |   `number`	| **Required.** The number of files to return in the pull request list files query. Used when getting data on the remote PR's files. |
+
+#### `logger`
+*[Logger](https://www.npmjs.com/package/@types/pino)* <br>
+The default logger is [Pino](https://github.com/pinojs/pino). You can plug in any logger that conforms to [Pino's interface](https://www.npmjs.com/package/@types/pino)
+
+
+#### Exceptions
+The core-library will throw an exception if the [GitHub V3 API](https://developer.github.com/v3/) returns an error response, or if the response data format did not come back as expected. <br>
 
 ### createPullRequest(options)
 

package.json

@@ -44,6 +44,7 @@
     "@octokit/rest": "^18.0.1",
     "@types/yargs": "^15.0.5",
     "async-retry": "^1.3.1",
+    "diff": "^4.0.2",
     "glob": "^7.1.6",
     "pino": "^6.3.2",
     "yargs": "^16.0.0"
@@ -53,6 +54,7 @@
     "@microsoft/api-extractor": "^7.8.10",
     "@types/async-retry": "^1.4.2",
     "@types/chai": "^4.2.11",
+    "@types/diff": "^4.0.2",
     "@types/mocha": "^8.0.0",
     "@types/node": "^14.0.20",
     "@types/pino": "^6.3.0",

src/default-options-handler.ts

@@ -12,10 +12,16 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import {CreatePullRequest, CreatePullRequestUserOptions} from './types';
+import {
+  CreatePullRequest,
+  CreatePullRequestUserOptions,
+  CreateReviewComment,
+  CreateReviewCommentUserOptions,
+} from './types';
 
 const DEFAULT_BRANCH_NAME = 'code-suggestions';
 const DEFAULT_PRIMARY_BRANCH = 'master';
+const DEFAULT_PAGE_SIZE = 100;
 
 /**
  * Add defaults to GitHub Pull Request options.
@@ -25,7 +31,7 @@ const DEFAULT_PRIMARY_BRANCH = 'master';
  * @param {PullRequestUserOptions} options the user-provided github pull request options
  * @returns {CreatePullRequest} git hub context with defaults applied
  */
-function addPullRequestDefaults(
+export function addPullRequestDefaults(
   options: CreatePullRequestUserOptions
 ): CreatePullRequest {
   const pullRequestSettings: CreatePullRequest = {
@@ -46,4 +52,23 @@ function addPullRequestDefaults(
   return pullRequestSettings;
 }
 
-export {addPullRequestDefaults};
+/**
+ * Format user input for pull request review comments
+ * @param options The user's options input for review comments
+ * @returns the formatted version of user input for pull request review comments
+ */
+export function addReviewCommentsDefaults(
+  options: CreateReviewCommentUserOptions
+): CreateReviewComment {
+  const createReviewComment: CreateReviewComment = {
+    repo: options.repo,
+    owner: options.owner,
+    pullNumber: options.pullNumber,
+    // if zero set as 0
+    pageSize:
+      options.pageSize === null || options.pageSize === undefined
+        ? DEFAULT_PAGE_SIZE
+        : options.pageSize,
+  };
+  return createReviewComment;
+}

src/github-handler/comment-handler/get-hunk-scope-handler/github-patch-text-handler.ts

@@ -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;
+}

src/github-handler/comment-handler/get-hunk-scope-handler/index.ts

@@ -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';

src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler.ts

@@ -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;
+  }
+}