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
 // required form
-export const suggestions = (user: string) =>
+export const explanations = (user: string) =>
     `@${user} I noticed these differences from the required 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

@@ -2,7 +2,7 @@ import * as Comments from "./comments";
 import * as urls from "./urls";
 import { PrInfo, BotNotFail, FileInfo } from "./pr-info";
 import { CIResult } from "./util/CIResult";
-import { ReviewInfo, Suggestion } from "./pr-info";
+import { ReviewInfo, Explanation } from "./pr-info";
 import { noNullish, flatten, unique, sameUser, daysSince, sha256, min } from "./util/util";
 
 type ColumnName =
@@ -51,7 +51,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(pr_number: number): Actions {
         targetColumn: "Other",
         labels: [],
         responseComments: [],
-        suggestions: [],
+        explanations: [],
         shouldClose: false,
         shouldMerge: false,
         shouldUpdateLabels: true,
@@ -79,7 +79,7 @@ function createEmptyActions(prNumber: number): Actions {
         pr_number: prNumber,
         labels: [],
         responseComments: [],
-        suggestions: [],
+        explanations: [],
         shouldClose: false,
         shouldMerge: false,
         shouldUpdateLabels: false,
@@ -302,9 +302,9 @@ export function process(prInfo: BotNotFail,
     // 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
     if (!(info.hasChangereqs || info.approvedBy.includes("owner") || info.approvedBy.includes("maintainer"))) {
@@ -440,9 +440,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`,
@@ -473,15 +470,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(``,
@@ -520,6 +508,9 @@ function createWelcomeComment(info: ExtendedPrInfo, post: (c: Comments.Comment)
     display(` * ${emoji(info.ciResult === 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`);
@@ -540,7 +531,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

@@ -31,7 +31,7 @@ export async function executePrActions(actions: Actions, info: PRQueryResult, dr
         ...await getMutationsForProjectChanges(actions, pr),
         ...getMutationsForComments(actions, pr.id, botComments),
         ...getMutationsForCommentRemovals(actions, botComments),
-        ...getMutationsForSuggestions(actions, pr),
+        ...getMutationsForExplanations(actions, pr),
         ...getMutationsForChangingPRState(actions, pr),
     ]);
     if (!dry) {
@@ -113,26 +113,26 @@ 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 }) =>
+        ...actions.explanations.map(({ path, startLine, endLine, body }) =>
             createMutation(addPullRequestReviewThread, {
                 input: {
                     pullRequestId: pr.id,
                     path,
                     startLine: startLine === endLine ? undefined : startLine,
                     line: endLine,
-                    body: "```suggestion\n" + text + "```",
+                    body,
                 }
             })
         ),
         createMutation(submitPullRequestReview, {
             input: {
                 pullRequestId: pr.id,
-                body: comments.suggestions(pr.author.login),
+                body: comments.explanations(pr.author.login),
                 event: "COMMENT",
             }
         }),

src/pr-info.ts

@@ -70,14 +70,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 required 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 = {
@@ -206,7 +205,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
@@ -241,8 +240,8 @@ export async function deriveStateForPR(
     const refs = {
         head: headCommit.oid,
         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(
@@ -391,26 +390,26 @@ 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 tester = configSuspicious[basename];
-    if (!tester) return { suspect: `edited` };
+    if (!tester) return { body: `edited` };
     const newText = await getContents("head");
-    if (newText === undefined) return { suspect: `couldn't fetch contents` };
+    if (newText === undefined) return { body: `couldn't fetch contents` };
     return tester(newText, getContents);
 });
 configSuspicious["OTHER_FILES.txt"] = async newText =>
     // not empty
-    (newText.length === 0) ? { suspect: "empty" }
+    (newText.length === 0) ? { body: "empty" }
     : undefined;
 configSuspicious["package.json"] = makeJsonCheckerFromCore(
     { private: true },
@@ -446,23 +445,20 @@ configSuspicious["tsconfig.json"] = makeJsonCheckerFromCore(
 function makeJsonCheckerFromCore(requiredForm: any, ignoredKeys: string[], requiredFormUrl: string) {
     return async (newText: string, getContents: GetContents) => {
         let suggestion: any;
-        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 newJson = jsonDiff.deepClone(suggestion);
         jsonDiff.applyPatch(newJson, ignoredKeys.map(path => ({ op: "remove", path })));
         const towardsIt = jsonDiff.deepClone(requiredForm);
         // Getting closer to the required form relative to master isn't
         // 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(newJson, towardsIt));
-        return {
-            suspect: vsMaster.suspect,
-            suggestion: makeJsonSuggestion(),
-        };
+        return makeJsonSuggestion();
 
         // Apply any preexisting diffs to towardsIt
         async function ignoreExistingDiffs(ref: keyof Refs) {
@@ -508,7 +504,7 @@ function makeJsonCheckerFromCore(requiredForm: any, ignoredKeys: string[], requi
             return {
                 startLine,
                 endLine,
-                text: suggestionLines.join(""),
+                body: vsMaster!.suspect + "\n```suggestion\n" + suggestionLines.join("") + "```",
             };
         }
     };