Repository: DefinitelyTyped/dt-mergebot
Pull request #329 (2021-01-14 18:16:03)

Check for failure before deriveStateForPR() vs. after
What do you think about factoring `BotFail` out of `BotResult`?

Instead of checking for `!prInfo` in `deriveStateForPR()` and also checking `state.type === "fail"` immediately afterwards (to narrow `BotResult` -> `BotNotFail`), replace the second check (`state.type === "fail"`) with `!prInfo`, move it ahead of `deriveStateForPR()` and call `deriveStateForPR()` on the non-null `prInfo`?

Also, instead of passing the whole GraphQL result to `executePrActions()` and then non-null asserting `info.repository!.pullRequest!`, just pass the non-null pull request data, like `deriveStateForPR()`.

No change in behavior, just cleave `BotFail` from `BotNotFail` and handle them separately, before and after `deriveStateForPR()`. Also makes the `executePrActions()` signature (slightly) stricter.


Patch:
diff --git a/src/commands/create-fixture.ts b/src/commands/create-fixture.ts
index 534f785..12589a5 100644
--- a/src/commands/create-fixture.ts
+++ b/src/commands/create-fixture.ts
@@ -1,7 +1,9 @@
 import * as computeActions from "../compute-pr-actions";
 import { deriveStateForPR, BotResult, queryPRInfo } from "../pr-info";
+import { ApolloQueryResult } from "@apollo/client/core";
 import { writeFileSync, mkdirSync, existsSync, readJsonSync } from "fs-extra";
 import { join } from "path";
+import { PR } from "../queries/schema/PR";
 import { fetchFile } from "../util/fetchFile";
 import { getMonthlyDownloadCount } from "../util/npm";
 import { scrubDiagnosticDetails } from "../util/util";
@@ -21,7 +23,7 @@ export default async function main(directory: string, overwriteInfo: boolean) {
   if (overwriteInfo || !existsSync(jsonFixturePath)) {
     writeJsonSync(jsonFixturePath, await queryPRInfo(prNumber));
   }
-  const response = readJsonSync(jsonFixturePath);
+  const response: ApolloQueryResult<PR> = readJsonSync(jsonFixturePath);
 
   const filesJSONPath = join(fixturePath, "_files.json");
   const filesFetched: {[expr: string]: string | undefined} = {};
@@ -31,8 +33,14 @@ export default async function main(directory: string, overwriteInfo: boolean) {
 
   const shouldOverwrite = (file: string) => overwriteInfo || !existsSync(file);
 
+  const prInfo = response.data.repository?.pullRequest;
+  if (!prInfo) {
+    console.error(`Could not get PR info for ${directory}, is the number correct?`);
+    return;
+  }
+
   const derivedInfo = await deriveStateForPR(
-    response,
+    prInfo,
     shouldOverwrite(filesJSONPath) ? initFetchFilesAndWriteToFile() : getFilesFromFile,
     shouldOverwrite(downloadsJSONPath) ? initGetDownloadsAndWriteToFile() : getDownloadsFromFile,
     shouldOverwrite(derivedFixturePath) ? undefined : getTimeFromFile(),
@@ -40,14 +48,12 @@ export default async function main(directory: string, overwriteInfo: boolean) {
 
   writeJsonSync(derivedFixturePath, derivedInfo);
 
-  if (derivedInfo.type === "fail") return;
-
   const resultFixturePath = join(fixturePath, "result.json");
   const actions = computeActions.process(derivedInfo);
   writeJsonSync(resultFixturePath, actions);
 
   const mutationsFixturePath = join(fixturePath, "mutations.json");
-  const mutations = await executePrActions(actions, response.data, /*dry*/ true);
+  const mutations = await executePrActions(actions, prInfo, /*dry*/ true);
   writeJsonSync(mutationsFixturePath, mutations);
 
   console.log(`Recorded`);
diff --git a/src/compute-pr-actions.ts b/src/compute-pr-actions.ts
index 33d0d77..22ce7ed 100644
--- a/src/compute-pr-actions.ts
+++ b/src/compute-pr-actions.ts
@@ -1,6 +1,6 @@
 import * as Comments from "./comments";
 import * as urls from "./urls";
-import { PrInfo, BotNotFail, FileInfo } from "./pr-info";
+import { PrInfo, BotResult, FileInfo } from "./pr-info";
 import { CIResult } from "./util/CIResult";
 import { ReviewInfo } from "./pr-info";
 import { noNullish, flatten, unique, sameUser, daysSince, min, sha256, abbrOid } from "./util/util";
@@ -231,7 +231,7 @@ function extendPrInfo(info: PrInfo): ExtendedPrInfo {
 
 }
 
-export function process(prInfo: BotNotFail,
+export function process(prInfo: BotResult,
                         extendedCallback: (info: ExtendedPrInfo) => void = _i => {}): Actions {
     if (prInfo.type === "remove") {
         if (prInfo.isDraft) {
diff --git a/src/execute-pr-actions.ts b/src/execute-pr-actions.ts
index e7ad60e..b6b14de 100644
--- a/src/execute-pr-actions.ts
+++ b/src/execute-pr-actions.ts
@@ -1,6 +1,6 @@
 import { MutationOptions } from "@apollo/client/core";
 import * as schema from "@octokit/graphql-schema/schema";
-import { PR as PRQueryResult, PR_repository_pullRequest } from "./queries/schema/PR";
+import { PR_repository_pullRequest } from "./queries/schema/PR";
 import { Actions, LabelNames, LabelName } from "./compute-pr-actions";
 import { createMutation, client } from "./graphql-client";
 import { getProjectBoardColumns, getLabels } from "./util/cachedQueries";
@@ -11,8 +11,7 @@ import * as comment from "./util/comment";
 // https://github.com/DefinitelyTyped/DefinitelyTyped/projects/5
 const ProjectBoardNumber = 5;
 
-export async function executePrActions(actions: Actions, info: PRQueryResult, dry?: boolean) {
-    const pr = info.repository!.pullRequest!;
+export async function executePrActions(actions: Actions, pr: PR_repository_pullRequest, dry?: boolean) {
     const botComments: ParsedComment[] = getBotComments(pr);
     const mutations = noNullish([
         ...await getMutationsForLabels(actions, pr),
diff --git a/src/pr-info.ts b/src/pr-info.ts
index 5ed8264..3eef230 100644
--- a/src/pr-info.ts
+++ b/src/pr-info.ts
@@ -1,5 +1,5 @@
 import { GetPRInfo } from "./queries/pr-query";
-import { PR as PRQueryResult, PR_repository_pullRequest as GraphqlPullRequest,
+import { PR_repository_pullRequest as GraphqlPullRequest,
          PR_repository_pullRequest,
          PR_repository_pullRequest_commits_nodes_commit_checkSuites,
          PR_repository_pullRequest_timelineItems,
@@ -12,7 +12,6 @@ import { CIResult } from "./util/CIResult";
 import { PullRequestReviewState, PullRequestState, CommentAuthorAssociation, CheckConclusionState } from "./queries/schema/graphql-global-types";
 import { getMonthlyDownloadCount } from "./util/npm";
 import { client } from "./graphql-client";
-import { ApolloQueryResult } from "@apollo/client/core";
 import { fetchFile as defaultFetchFile } from "./util/fetchFile";
 import { noNullish, findLast, sameUser, authorNotBot, max, abbrOid } from "./util/util";
 import * as comment from "./util/comment";
@@ -28,13 +27,6 @@ export type PopularityLevel =
     | "Popular"
     | "Critical";
 
-// Complete failure, won't be passed to `process` (no PR found)
-interface BotFail {
-    readonly type: "fail";
-    readonly now: string;
-    readonly message: string;
-}
-
 // Some error found, will be passed to `process` to report in a comment
 interface BotError {
     readonly type: "error";
@@ -148,13 +140,11 @@ export interface PrInfo {
     readonly reviews: readonly ReviewInfo[];
 }
 
-export type BotNotFail =
+export type BotResult =
     | PrInfo
     | BotError
     | BotEnsureRemovedFromProject;
 
-export type BotResult = BotNotFail | BotFail;
-
 function getHeadCommit(pr: GraphqlPullRequest) {
     return pr.commits.nodes?.find(c => c?.commit.oid === pr.headRefOid)?.commit;
 }
@@ -192,14 +182,11 @@ export async function queryPRInfo(prNumber: number) {
 
 // The GQL response => Useful data for us
 export async function deriveStateForPR(
-    info: ApolloQueryResult<PRQueryResult>,
+    prInfo: PR_repository_pullRequest,
     fetchFile = defaultFetchFile,
     getDownloads = getMonthlyDownloadCount,
     now = new Date().toISOString(),
 ): Promise<BotResult>  {
-    const prInfo = info.data.repository?.pullRequest;
-
-    if (!prInfo) return botFail(`No PR with this number exists, (${JSON.stringify(info)})`);
     if (prInfo.author == null) return botError(prInfo.number, "PR author does not exist");
 
     if (prInfo.isDraft) return botEnsureRemovedFromProject(prInfo.number, "PR is a draft", true);
@@ -251,12 +238,8 @@ export async function deriveStateForPR(
         ...getCIResult(headCommit.checkSuites)
     };
 
-    function botFail(message: string): BotFail {
-        return { type: "fail", now, message };
-    }
-
     function botError(pr_number: number, message: string): BotError {
-        return { type: "error", now, message, pr_number, author: prInfo?.author?.login };
+        return { type: "error", now, message, pr_number, author: prInfo.author?.login };
     }
 
     function botEnsureRemovedFromProject(pr_number: number, message: string, isDraft: boolean): BotEnsureRemovedFromProject {
diff --git a/src/pr-trigger.ts b/src/pr-trigger.ts
index 3aa5617..3eb47ba 100644
--- a/src/pr-trigger.ts
+++ b/src/pr-trigger.ts
@@ -141,21 +141,21 @@ export async function httpTrigger(context: Context, req: HttpRequest) {
 
     // Generate the info for the PR from scratch
     const info = await queryPRInfo(prNumber);
-    const state = await deriveStateForPR(info);
+    const prInfo = info.data.repository?.pullRequest;
 
     // If it didn't work, bail early
-    if (state.type === "fail") {
-        const isIssueNotPR = state.message.startsWith("No PR with this number exists") && "issue" in webhook;
+    if (!prInfo) {
+        const isIssueNotPR = "issue" in webhook;
         if (isIssueNotPR) {
             context.res = {
                 status: 204,
                 body: `NOOPing due to ${prNumber} not being a PR`
             };
         } else {
-            context.log.error(`Failed because of: ${state.message}`);
+            context.log.error(`No PR with this number exists, (${JSON.stringify(info)})`);
             context.res = {
                 status: 422,
-                body: `Failed because of: ${state.message}`
+                body: `No PR with this number exists, (${JSON.stringify(info)})`
             };
         }
 
@@ -163,10 +163,11 @@ export async function httpTrigger(context: Context, req: HttpRequest) {
     }
 
     // Convert the info to a set of actions for the bot
+    const state = await deriveStateForPR(prInfo);
     const actions = computeActions(state);
 
     // Act on the actions
-    await executePrActions(actions, info.data);
+    await executePrActions(actions, prInfo);
 
     // We are responding real late in the process, so it might show
     // as a timeout in GH a few times (e.g. after GH/DT/NPM lookups)
diff --git a/src/run.ts b/src/run.ts
index 1012380..02f0a36 100755
--- a/src/run.ts
+++ b/src/run.ts
@@ -70,13 +70,14 @@ const start = async function () {
         // Generate the info for the PR from scratch
         const info = await queryPRInfo(pr);
         if (args["show-raw"]) show("Raw Query Result", info);
-        const state = await deriveStateForPR(info);
-        if (args["show-basic"]) show("Basic PR Info", state);
+        const prInfo = info.data.repository?.pullRequest;
         // If it didn't work, bail early
-        if (state.type === "fail") {
-            console.error(`  Failed because of: ${state.message}`);
+        if (!prInfo) {
+            console.error(`  No PR with this number exists, (${JSON.stringify(info)})`);
             continue;
         }
+        const state = await deriveStateForPR(prInfo);
+        if (args["show-basic"]) show("Basic PR Info", state);
         // Show errors in log but keep processing to show in a comment too
         if (state.type === "error") console.error(`  Error: ${state.message}`);
         // Show other messages too
@@ -86,7 +87,7 @@ const start = async function () {
             args["show-extended"] ? i => show("Extended Info", i) : undefined);
         if (args["show-actions"]) show("Actions", actions);
         // Act on the actions
-        const mutations = await executePrActions(actions, info.data, args.dry);
+        const mutations = await executePrActions(actions, prInfo, args.dry);
         if (args["show-mutations"] ?? args.dry) show("Mutations", mutations);
     }
     if (args.dry || !args.cleanup) return;


Test patch:
diff --git a/src/_tests/fixturedActions.test.ts b/src/_tests/fixturedActions.test.ts
index 7821ca4..f43bb78 100644
--- a/src/_tests/fixturedActions.test.ts
+++ b/src/_tests/fixturedActions.test.ts
@@ -1,8 +1,10 @@
+import { ApolloQueryResult } from "@apollo/client/core";
 import { readdirSync, readJsonSync } from "fs-extra";
 import { join } from "path";
 import { toMatchFile } from "jest-file-snapshot";
 import { process } from "../compute-pr-actions";
 import { deriveStateForPR, BotResult } from "../pr-info";
+import { PR } from "../queries/schema/PR";
 import { scrubDiagnosticDetails } from "../util/util";
 import * as cachedQueries from "./cachedQueries.json";
 jest.mock("../util/cachedQueries", () => ({
@@ -29,25 +31,26 @@ async function testFixture(dir: string) {
 
   const JSONString = (value: any) => scrubDiagnosticDetails(JSON.stringify(value, null, "  ") + "\n");
 
-  const response = readJsonSync(responsePath);
+  const response: ApolloQueryResult<PR> = readJsonSync(responsePath);
   const files = readJsonSync(filesPath);
   const downloads = readJsonSync(downloadsPath);
 
+  const prInfo = response.data.repository?.pullRequest;
+  if (!prInfo) throw new Error("Should never happen");
+
   const derived = await deriveStateForPR(
-    response,
+    prInfo,
     (expr: string) => Promise.resolve(files[expr] as string),
     (name: string, _until?: Date) => name in downloads ? downloads[name] : 0,
     (readJsonSync(derivedPath) as BotResult).now
   );
 
-  if (derived.type === "fail") throw new Error("Should never happen");
-
   const action = process(derived);
 
   expect(JSONString(action)).toMatchFile(resultPath);
   expect(JSONString(derived)).toMatchFile(derivedPath);
 
-  const mutations = await executePrActions(action, response.data, /*dry*/ true);
+  const mutations = await executePrActions(action, prInfo, /*dry*/ true);
   expect(JSONString(mutations)).toMatchFile(mutationsPath);
 }