Suggest the required form

Author: jablko

package.json

@@ -25,6 +25,7 @@
     "idembot": "^0.0.12",
     "moment": "^2.27.0",
     "node-fetch": "^1.7.3",
+    "prettier": "^2.2.0",
     "prettyjson": "^1.2.1",
     "request": "^2.88.2",
     "tslib": "^1.13.0",

src/comments.ts

@@ -91,3 +91,8 @@ I can't [accept a merge request](${uri}) until the PR has a green CI and was app
 
 Thanks, and happy typing!`
 });
+
+export const Suggestions = (user: string) => ({
+    tag: "suggestions",
+    status: `@${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 (unless it's obvious) and a maintainer will take a look. Thanks!`,
+});

src/compute-pr-actions.ts

@@ -1,7 +1,7 @@
 import * as Comments from "./comments";
 import { PrInfo, BotError, BotEnsureRemovedFromProject, BotNoPackages, FileInfo } from "./pr-info";
 import { CIResult } from "./util/CIResult";
-import { ReviewInfo } from "./pr-info";
+import { ReviewInfo, Suggestion } from "./pr-info";
 import { noNulls, flatten, unique, sameUser, daysSince, sha256 } from "./util/util";
 
 type ColumnName =
@@ -44,6 +44,7 @@ export interface Actions {
     targetColumn?: ColumnName;
     labels: LabelName[];
     responseComments: Comments.Comment[];
+    suggestions: { [path: string]: Suggestion };
     shouldClose: boolean;
     shouldMerge: boolean;
     shouldUpdateLabels: boolean;
@@ -58,6 +59,7 @@ function createDefaultActions(pr_number: number): Actions {
         targetColumn: "Other",
         labels: [],
         responseComments: [],
+        suggestions: {},
         shouldClose: false,
         shouldMerge: false,
         shouldUpdateLabels: true,
@@ -72,6 +74,7 @@ function createEmptyActions(prNumber: number): Actions {
         pr_number: prNumber,
         labels: [],
         responseComments: [],
+        suggestions: {},
         shouldClose: false,
         shouldMerge: false,
         shouldUpdateLabels: false,
@@ -305,6 +308,15 @@ export function process(prInfo: PrInfo | BotEnsureRemovedFromProject | BotNoPack
     // Update intro comment
     post({ tag: "welcome", status: createWelcomeComment(info) });
 
+    // Propagate suggestions into actions
+    for (const pkg of info.pkgInfo) {
+        for (const file of pkg.files) {
+            if (file.suggestion) {
+                context.suggestions[file.path] = file.suggestion;
+            }
+        }
+    }
+
     // Ping reviewers when needed
     if (!(info.hasChangereqs || info.approvedBy.includes("owner") || info.approvedBy.includes("maintainer"))) {
         if (info.noOtherOwners) {

src/execute-pr-actions.ts

@@ -4,6 +4,7 @@ import { createMutation, mutate } from "./graphql-client";
 import { getProjectBoardColumns, getLabels } from "./util/cachedQueries";
 import { noNulls, flatten } from "./util/util";
 import * as comment from "./util/comment";
+import * as comments from "./comments";
 
 // https://github.com/DefinitelyTyped/DefinitelyTyped/projects/5
 const ProjectBoardNumber = 5;
@@ -19,6 +20,9 @@ const addProjectCard = `mutation($input: AddProjectCardInput!) { addProjectCard(
 const moveProjectCard = `mutation($input: MoveProjectCardInput!) { moveProjectCard(input: $input) { clientMutationId } }`;
 export const deleteProjectCard = `mutation($input: DeleteProjectCardInput!) { deleteProjectCard(input: $input) { clientMutationId } }`;
 
+const addPullRequestReviewThread = 'mutation($input: AddPullRequestReviewThreadInput!) { addPullRequestReviewThread(input: $input) { clientMutationId } }';
+const submitPullRequestReview = 'mutation($input: SubmitPullRequestReviewInput!) { submitPullRequestReview(input: $input) { clientMutationId } }';
+
 export async function executePrActions(actions: Actions, info: PRQueryResult, dry?: boolean) {
     const pr = info.repository?.pullRequest!;
     const botComments: ParsedComment[] = getBotComments(pr);
@@ -27,6 +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),
         ...getMutationsForChangingPRState(actions, pr),
     ]);
     if (!dry) {
@@ -106,6 +111,32 @@ function getMutationsForCommentRemovals(actions: Actions, botComments: ParsedCom
     });
 }
 
+function getMutationsForSuggestions(actions: Actions, pr: PR_repository_pullRequest) {
+    if (pr.reviews?.nodes?.find(review => review?.author?.login === "typescript-bot")) return [];
+    const mutations = Object.entries(actions.suggestions).map(([path, { startLine, line, suggestion }]) =>
+        createMutation(addPullRequestReviewThread, {
+            input: {
+                pullRequestId: pr.id,
+                path,
+                startLine: startLine === line ? undefined : startLine,
+                line,
+                body: "```suggestion\n" + suggestion,
+            }
+        })
+    );
+    if (mutations.length === 0) return [];
+    return [
+        ...mutations,
+        createMutation(submitPullRequestReview, {
+            input: {
+                pullRequestId: pr.id,
+                body: comment.make(comments.Suggestions(pr.author!.login)),
+                event: "COMMENT",
+            }
+        }),
+    ];
+}
+
 function getMutationsForChangingPRState(actions: Actions, pr: PR_repository_pullRequest) {
     return [
         actions.shouldMerge

src/pr-info.ts

@@ -20,6 +20,7 @@ import { noNulls, notUndefined, findLast, forEachReverse, sameUser, authorNotBot
 import * as comment from "./util/comment";
 import * as HeaderParser from "definitelytyped-header-parser";
 import * as jsonDiff from "fast-json-patch";
+import * as prettier from "prettier";
 import { PullRequestState } from "./schema/graphql-global-types";
 
 const CriticalPopularityThreshold = 5_000_000;
@@ -71,9 +72,16 @@ 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"
+    suspect?: string, // reason for a file being "package-meta" rather than "package-meta-ok"
+    suggestion?: Suggestion,
 };
 
+export interface Suggestion {
+    readonly startLine: number;
+    readonly line: number;
+    readonly suggestion: string;
+}
+
 export type ReviewInfo = {
     type: string,
     reviewer: string,
@@ -348,21 +356,21 @@ async function categorizeFile(path: string, contents: (oid?: string) => Promise<
         case "md": return [pkg, { path, kind: "markdown" }];
         default:
             const suspect = await configSuspicious(path, contents);
-            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: (oid?: string) => Promise<string | undefined>): Promise<string | undefined>;
-    [basename: string]: (text: string, oldText?: string) => string | undefined;
+    (path: string, getContents: (oid?: string) => Promise<string | undefined>): Promise<{ suspect: string, sugestion?: Suggestion } | undefined>;
+    [basename: string]: (text: string, oldText?: string) => { suspect: string, suggestion?: Suggestion } | undefined;
 };
 const configSuspicious = <ConfigSuspicious>(async (path, getContents) => {
     const basename = path.replace(/.*\//, "");
-    if (!(basename in configSuspicious)) return `edited`;
+    if (!(basename in configSuspicious)) return { suspect: `edited` };
     const text = await getContents();
-    if (text === undefined) return `couldn't fetch contents`;
+    if (text === undefined) return { suspect: `couldn't fetch contents` };
     const tester = configSuspicious[basename];
-    let suspect: string | undefined;
+    let suspect;
     if (tester.length === 1) {
         suspect = tester(text);
     } else {
@@ -373,7 +381,7 @@ const configSuspicious = <ConfigSuspicious>(async (path, getContents) => {
 });
 configSuspicious["OTHER_FILES.txt"] = contents =>
     // not empty
-    (contents.length === 0) ? "empty"
+    (contents.length === 0) ? { suspect: "empty" }
     : undefined;
 configSuspicious["package.json"] = makeJsonCheckerFromCore(
     { private: true },
@@ -404,22 +412,48 @@ configSuspicious["tsconfig.json"] = makeJsonCheckerFromCore(
 // to it, ignoring some keys (JSON Patch paths).  The ignored properties are in most cases checked
 // elsewhere (dtslint), and in some cases they are irrelevant.
 function makeJsonCheckerFromCore(requiredForm: any, ignoredKeys: string[]) {
-    const diffFromReq = (text: string) => {
-        let json: any;
-        try { json = JSON.parse(text); } catch (e) { return "couldn't parse json"; }
-        jsonDiff.applyPatch(json, ignoredKeys.map(path => ({ op: "remove", path })));
-        try { return jsonDiff.compare(requiredForm, json); } catch (e) { return "couldn't diff json" };
-    };
     return (contents: string, oldText?: string) => {
-        const newDiff = diffFromReq(contents);
-        if (typeof newDiff === "string") return newDiff;
+        const diffFromReq = (json: any) => {
+            jsonDiff.applyPatch(json, ignoredKeys.map(path => ({ op: "remove", path })));
+            return jsonDiff.compare(requiredForm, json);
+        };
+        let newJson;
+        try { newJson = JSON.parse(contents); } catch (e) { return { suspect: "couldn't parse json" }; }
+        const suggestion = jsonDiff.deepClone(newJson);
+        const newDiff = diffFromReq(newJson);
         if (newDiff.length === 0) return undefined;
-        if (!oldText) return `not the required form \`${JSON.stringify(newDiff)}\``;
-        const oldDiff = diffFromReq(oldText);
-        if (typeof oldDiff === "string") return oldDiff;
-        const notRemove = jsonDiff.compare(oldDiff, newDiff).filter(({ op }) => op !== "remove");
-        if (notRemove.length === 0) return undefined;
-        return `not the required form and not moving towards it \`${JSON.stringify(notRemove.map(({ path }) => newDiff[Number(path.slice(1))]))}\``;
+        const towardsIt = jsonDiff.deepClone(requiredForm);
+        if (oldText) {
+            let oldJson;
+            try { oldJson = JSON.parse(oldText); } catch (e) { return { suspect: "couldn't parse json" }; }
+            const oldDiff = diffFromReq(oldJson);
+            const notRemove = jsonDiff.compare(oldDiff, newDiff).filter(({ op }) => op !== "remove");
+            if (notRemove.length === 0) return undefined;
+            jsonDiff.applyPatch(newDiff, notRemove.map(({ path }) => ({ op: "remove", path })));
+            jsonDiff.applyPatch(towardsIt, newDiff.filter(({ op }) => op));
+        }
+        jsonDiff.applyPatch(suggestion, jsonDiff.compare(newJson, towardsIt));
+        // Suggest the different lines to the author
+        const suggestionLines = (
+            Object.keys(suggestion).length <= 1
+                ? prettier.format(JSON.stringify(suggestion), { tabWidth: 4, filepath: ".json" })
+                : JSON.stringify(suggestion, undefined, 4) + "\n"
+        ).split(/^/m);
+        const lines = contents.split(/^/m);
+        let i = 0;
+        while (suggestionLines[i].trim() === lines[i].trim()) i++;
+        let j = 1;
+        while (suggestionLines[suggestionLines.length - j].trim() === lines[lines.length - j].trim()) j++;
+        return {
+            suspect: oldText
+                ? "not the required form and not moving towards it"
+                : "not the required form",
+            suggestion: {
+                startLine: i + 1,
+                line: lines.length - j + 1,
+                suggestion: suggestionLines.slice(i, 1 - j || undefined).join(""),
+            },
+        };
     };
 }