diff --git a/src/comments.ts b/src/comments.ts index 02754f6b1..ae853e492 100644 --- a/src/comments.ts +++ b/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!`; diff --git a/src/compute-pr-actions.ts b/src/compute-pr-actions.ts index e225c2649..549134bfd 100644 --- a/src/compute-pr-actions.ts +++ b/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"))) { @@ -531,9 +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.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 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})`); } diff --git a/src/execute-pr-actions.ts b/src/execute-pr-actions.ts index ddfbe39a9..ac355068a 100644 --- a/src/execute-pr-actions.ts +++ b/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", } }), diff --git a/src/pr-info.ts b/src/pr-info.ts index 84232a239..2e2465aec 100644 --- a/src/pr-info.ts +++ b/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; + [basename: string]: (newText: string, getContents: GetContents) => Promise; } const 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,7 +445,7 @@ 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: ${e.message}` }; } + try { suggestion = JSON.parse(newText); } catch (e) { 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); @@ -454,15 +453,12 @@ function makeJsonCheckerFromCore(requiredForm: any, ignoredKeys: string[], requi // 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("") + "```", }; } };