Put explanations next to suggestions

Author: jablko

src/comments.ts

@@ -125,5 +125,5 @@ I'll bump it to the DT maintainer queue. Thank you for your patience, @${author}
 
 // Introduction to the review when config files diverge from the
 // expected form
-export const suggestions = (user: string) =>
+export const explanations = (user: string) =>
     `@${user} I noticed these differences from the expected form. If you can revise your changes to avoid them, so much the better! Otherwise please reply with explanations why they're needed and a maintainer will take a look. Thanks!`;

src/compute-pr-actions.ts

@@ -1,7 +1,7 @@
 import * as Comments from "./comments";
 import * as urls from "./urls";
 import { PrInfo, BotResult, FileInfo } from "./pr-info";
-import { ReviewInfo, Suggestion } from "./pr-info";
+import { ReviewInfo, Explanation } from "./pr-info";
 import { noNullish, flatten, unique, sameUser, min, sha256, abbrOid } from "./util/util";
 import * as dayjs from "dayjs";
 import * as advancedFormat from "dayjs/plugin/advancedFormat";
@@ -52,7 +52,7 @@ export interface Actions {
     targetColumn?: ColumnName;
     labels: LabelName[];
     responseComments: Comments.Comment[];
-    suggestions: ({ path: string } & Suggestion)[];
+    explanations: ({ path: string } & Explanation)[];
     shouldClose: boolean;
     shouldMerge: boolean;
     shouldUpdateLabels: boolean;
@@ -65,7 +65,7 @@ function createDefaultActions(): Actions {
         targetColumn: "Other",
         labels: [],
         responseComments: [],
-        suggestions: [],
+        explanations: [],
         shouldClose: false,
         shouldMerge: false,
         shouldUpdateLabels: true,
@@ -78,7 +78,7 @@ function createEmptyActions(): Actions {
     return {
         labels: [],
         responseComments: [],
-        suggestions: [],
+        explanations: [],
         shouldClose: false,
         shouldMerge: false,
         shouldUpdateLabels: false,
@@ -294,9 +294,9 @@ export function process(prInfo: BotResult,
     // Update intro comment
     post({ tag: "welcome", status: createWelcomeComment(info, post) });
 
-    // Propagate suggestions into actions
-    context.suggestions = noNullish(flatten(info.pkgInfo.map(pkg => pkg.files.map(({ path, suggestion }) =>
-        suggestion && { path, ...suggestion }))));
+    // Propagate explanations into actions
+    context.explanations = noNullish(flatten(info.pkgInfo.map(pkg => pkg.files.map(({ path, suspect }) =>
+        suspect && { path, ...suspect }))));
 
     // Ping reviewers when needed
     const headCommitAbbrOid = abbrOid(info.headCommitOid);
@@ -433,9 +433,6 @@ function createWelcomeComment(info: ExtendedPrInfo, post: (c: Comments.Comment)
 
     const announceList = (what: string, xs: readonly string[]) => `${xs.length} ${what}${xs.length !== 1 ? "s" : ""}`;
     const usersToString = (users: string[]) => users.map(u => (info.isAuthor(u) ? "✎" : "") + "@" + u).join(", ");
-    const reviewLink = (f: FileInfo) =>
-        `[\`${f.path.replace(/^types\/(.*\/)/, "$1")}\`](${
-          urls.review(info.pr_number)}/${info.headCommitOid}#diff-${sha256(f.path)})`;
 
     display(``,
             `## ${announceList("package", info.packages)} in this PR`,
@@ -465,15 +462,6 @@ function createWelcomeComment(info: ExtendedPrInfo, post: (c: Comments.Comment)
         displayOwners("added", p.addedOwners);
         displayOwners("removed", p.deletedOwners);
         if (!info.authorIsOwner && p.owners.length >= 4 && p.addedOwners.some(info.isAuthor)) addedSelfToManyOwners++;
-
-        let showSuspects = false;
-        for (const file of p.files) {
-            if (!file.suspect) continue;
-            if (!showSuspects) display(`  - Config files to check:`);
-            display(`    - ${reviewLink(file)}: ${file.suspect}`);
-            showSuspects = true;
-        }
-
     }
     if (addedSelfToManyOwners > 0) {
         display(``,
@@ -512,6 +500,9 @@ function createWelcomeComment(info: ExtendedPrInfo, post: (c: Comments.Comment)
     display(` * ${emoji(info.ciResult === "pass")} Continuous integration tests have ${expectedResults}`);
 
     const approved = emoji(info.approved);
+    const reviewLink = (f: FileInfo) =>
+        `[\`${f.path.replace(/^types\/(.*\/)/, "$1")}\`](${
+          urls.review(info.pr_number)}/${info.headCommitOid}#diff-${sha256(f.path)})`;
 
     if (info.hasNewPackages) {
         display(` * ${approved} Only ${requiredApprover} can approve changes when there are new packages added`);
@@ -532,7 +523,9 @@ function createWelcomeComment(info: ExtendedPrInfo, post: (c: Comments.Comment)
     } else if (info.noOtherOwners) {
         display(` * ${approved} ${RequiredApprover} can merge changes when there are no other reviewers`);
     } else if (info.checkConfig) {
-        display(` * ${approved} ${RequiredApprover} needs to approve changes which affect module config files`);
+        const configFiles = flatten(info.pkgInfo.map(pkg => pkg.files.filter(({ kind }) => kind === "package-meta")));
+        const links = configFiles.map(reviewLink);
+        display(` * ${approved} ${RequiredApprover} needs to approve changes which affect module config files (${links.join(", ")})`);
     } else {
         display(` * ${approved} Only ${requiredApprover} can approve changes [without tests](${testsLink})`);
     }

src/execute-pr-actions.ts

@@ -18,7 +18,7 @@ export async function executePrActions(actions: Actions, pr: PR_repository_pullR
         ...await getMutationsForProjectChanges(actions, pr),
         ...getMutationsForComments(actions, pr.id, botComments),
         ...getMutationsForCommentRemovals(actions, botComments),
-        ...getMutationsForSuggestions(actions, pr),
+        ...getMutationsForExplanations(actions, pr),
         ...getMutationsForChangingPRState(actions, pr),
     ]);
     if (!dry) {
@@ -102,23 +102,28 @@ function getMutationsForCommentRemovals(actions: Actions, botComments: ParsedCom
     });
 }
 
-function getMutationsForSuggestions(actions: Actions, pr: PR_repository_pullRequest) {
-    // Suggestions will be empty if we already reviewed this head
-    if (actions.suggestions.length === 0) return [];
+function getMutationsForExplanations(actions: Actions, pr: PR_repository_pullRequest) {
+    // Explanations will be empty if we already reviewed this head
+    if (actions.explanations.length === 0) return [];
     if (!pr.author) throw new Error("Internal Error: expected to be checked");
     return [
-        ...actions.suggestions.map(({ path, startLine, endLine, text }) =>
-            createMutation<schema.AddPullRequestReviewThreadInput>("addPullRequestReviewThread", {
+        ...actions.explanations.map(({ path, startLine, endLine, body }) => endLine
+            ? createMutation<schema.AddPullRequestReviewThreadInput>("addPullRequestReviewThread", {
                 pullRequestId: pr.id,
                 path,
                 startLine: startLine === endLine ? undefined : startLine,
                 line: endLine,
-                body: "```suggestion\n" + text + "```",
+                body,
+            })
+            : createMutation<schema.AddPullRequestReviewCommentInput>("addPullRequestReviewComment", {
+                pullRequestId: pr.id,
+                path,
+                body,
             })
         ),
         createMutation<schema.SubmitPullRequestReviewInput>("submitPullRequestReview", {
             pullRequestId: pr.id,
-            body: comments.suggestions(pr.author.login),
+            body: comments.explanations(pr.author.login),
             event: "COMMENT",
         }),
     ];

src/pr-info.ts

@@ -58,14 +58,13 @@ type FileKind = "test" | "definition" | "markdown" | "package-meta" | "package-m
 export type FileInfo = {
     path: string,
     kind: FileKind,
-    suspect?: string, // reason for a file being "package-meta" rather than "package-meta-ok"
-    suggestion?: Suggestion, // The differences from the expected form, as GitHub suggestions
+    suspect?: Explanation // reason for a file being "package-meta" rather than "package-meta-ok"
 };
 
-export interface Suggestion {
-    readonly startLine: number;
-    readonly endLine: number;
-    readonly text: string;
+export interface Explanation {
+    readonly startLine?: number;
+    readonly endLine?: number;
+    readonly body: string;
 }
 
 export type ReviewInfo = {
@@ -188,7 +187,7 @@ export async function queryPRInfo(prNumber: number) {
 interface Refs {
     readonly head: string;
     readonly master: "master";
-    readonly latestSuggestions: string;
+    readonly latestExplanations: string;
 }
 
 // The GQL response => Useful data for us
@@ -220,8 +219,8 @@ export async function deriveStateForPR(
     const refs = {
         head: prInfo.headRefOid,
         master: "master",
-        // Exclude existing suggestions from subsequent reviews
-        latestSuggestions: max(noNullish(prInfo.reviews?.nodes).filter(review => !authorNotBot(review)), (a, b) =>
+        // Exclude existing explanations from subsequent reviews
+        latestExplanations: max(noNullish(prInfo.reviews?.nodes).filter(review => !authorNotBot(review)), (a, b) =>
             Date.parse(a.submittedAt) - Date.parse(b.submittedAt))?.commit?.oid,
     } as const;
     const pkgInfoEtc = await getPackageInfosEtc(
@@ -355,19 +354,19 @@ async function categorizeFile(path: string, getContents: GetContents): Promise<[
         case "md": return [pkg, { path, kind: "markdown" }];
         default: {
             const suspect = await configSuspicious(path, getContents);
-            return [pkg, { path, kind: suspect ? "package-meta" : "package-meta-ok", ...suspect }];
+            return [pkg, { path, kind: suspect ? "package-meta" : "package-meta-ok", suspect }];
         }
     }
 }
 
 interface ConfigSuspicious {
-    (path: string, getContents: GetContents): Promise<{ suspect: string, sugestion?: Suggestion } | undefined>;
-    [basename: string]: (newText: string, getContents: GetContents) => Promise<{ suspect: string, suggestion?: Suggestion } | undefined>;
+    (path: string, getContents: GetContents): Promise<Explanation | undefined>;
+    [basename: string]: (newText: string, getContents: GetContents) => Promise<Explanation | undefined>;
 }
 const configSuspicious = <ConfigSuspicious>(async (path, getContents) => {
     const basename = path.replace(/.*\//, "");
     const checker = configSuspicious[basename];
-    if (!checker) return { suspect: `edited` };
+    if (!checker) return { body: `edited` };
     const newText = await getContents("head");
     // Removing tslint.json, tsconfig.json, package.json and
     // OTHER_FILES.txt is checked by the CI. Specifics are in my commit
@@ -426,7 +425,7 @@ function makeChecker(expectedForm: any, expectedFormUrl: string, options?: { par
         if (options && "parse" in options) {
             suggestion = options.parse(newText);
         } else {
-            try { suggestion = JSON.parse(newText); } catch (e) { if (e instanceof SyntaxError) return { suspect: `couldn't parse json: ${e.message}` }; }
+            try { suggestion = JSON.parse(newText); } catch (e) { if (e instanceof SyntaxError) return { body: `couldn't parse json: ${e.message}` }; }
         }
         const newData = jsonDiff.deepClone(suggestion);
         if (options && "ignore" in options) options.ignore(newData);
@@ -435,15 +434,12 @@ function makeChecker(expectedForm: any, expectedFormUrl: string, options?: { par
         // suspect
         const vsMaster = await ignoreExistingDiffs("master");
         if (!vsMaster) return undefined;
-        if (vsMaster.done) return { suspect: vsMaster.suspect };
+        if (vsMaster.done) return { body: vsMaster.suspect };
         // whereas getting closer relative to existing suggestions means
         // no new suggestions
-        if (!await ignoreExistingDiffs("latestSuggestions")) return { suspect: vsMaster.suspect };
+        if (!await ignoreExistingDiffs("latestExplanations")) return { body: vsMaster.suspect };
         jsonDiff.applyPatch(suggestion, jsonDiff.compare(newData, towardsIt));
-        return {
-            suspect: vsMaster.suspect,
-            suggestion: makeSuggestion(),
-        };
+        return makeSuggestion();
 
         // Apply any preexisting diffs to towardsIt
         async function ignoreExistingDiffs(ref: keyof Refs) {
@@ -493,7 +489,7 @@ function makeChecker(expectedForm: any, expectedFormUrl: string, options?: { par
             return {
                 startLine,
                 endLine,
-                text: suggestionLines.join(""),
+                body: vsMaster!.suspect + "\n```suggestion\n" + suggestionLines.join("") + "```",
             };
         }
     };