Put explanations next to suggestions

Author: jablko

src/comments.ts

@@ -119,5 +119,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, BotError, BotEnsureRemovedFromProject, BotNoPackages, 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 } 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,
@@ -303,9 +303,9 @@ export function process(prInfo: PrInfo | BotEnsureRemovedFromProject | BotNoPack
     // Update intro comment
     post({ tag: "welcome", status: createWelcomeComment(info) });
 
-    // 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"))) {
@@ -531,9 +531,9 @@ function createWelcomeComment(info: ExtendedPrInfo) {
     } else if (info.noOtherOwners) {
         display(` * ${approved} ${RequiredApprover} can merge changes when there are no other reviewers`);
     } else if (info.editsConfig) {
-        display(` * ${approved} ${RequiredApprover} needs to approve changes which affect module config files`);
-        info.pkgInfo.forEach(pkg => pkg.files.forEach(file =>
-            file.suspect && display(`   - ${reviewLink(file)}: ${file.suspect}`)));
+        const links = noNullish(flatten(info.pkgInfo.map(pkg => pkg.files.map(file =>
+            file.suspect && reviewLink(file)))));
+        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

@@ -74,14 +74,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 = {
@@ -201,7 +200,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
@@ -236,8 +235,8 @@ export async function deriveStateForPR(
     const refs = {
         head: headCommit.oid,
         master: "master",
-        // Exclude existing suggestions from subsequent reviews
-        latestSuggestions: prInfo.reviews?.nodes?.reduce((latest, review) =>
+        // Exclude existing explanations from subsequent reviews
+        latestExplanations: prInfo.reviews?.nodes?.reduce((latest, review) =>
             review && !authorNotBot(review) && (
                 !latest?.submittedAt || review.submittedAt && new Date (review.submittedAt) > new Date(latest.submittedAt))
                     ? review : latest, null)?.commit?.oid,
@@ -393,26 +392,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) { return { suspect: "couldn't parse json" }; }
+        try { suggestion = JSON.parse(newText); } catch (e) { return { body: "couldn't parse json" }; }
         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) {
@@ -511,7 +507,7 @@ function makeJsonCheckerFromCore(requiredForm: any, ignoredKeys: string[], requi
             return {
                 startLine,
                 endLine,
-                text: suggestionLines.join(""),
+                body: vsMaster!.suspect + "\n```suggestion\n" + suggestionLines.join("") + "```",
             };
         }
     };