diff --git a/src/comments.ts b/src/comments.ts index cdca1775e..4f3cf17ad 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 // expected form -export const suggestions = (user: string) => +export const explanations = (user: string) => `@${user} I noticed these differences from the expected 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 9098e6878..f780a43d6 100644 --- a/src/compute-pr-actions.ts +++ b/src/compute-pr-actions.ts @@ -1,7 +1,7 @@ import * as Comments from "./comments"; import * as urls from "./urls"; import { PrInfo, BotResult, FileInfo } from "./pr-info"; -import { ReviewInfo, Suggestion } from "./pr-info"; +import { ReviewInfo, Explanation } from "./pr-info"; import { noNullish, flatten, unique, sameUser, min, sha256, abbrOid } from "./util/util"; import * as dayjs from "dayjs"; import * as advancedFormat from "dayjs/plugin/advancedFormat"; @@ -52,7 +52,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(): Actions { targetColumn: "Other", labels: [], responseComments: [], - suggestions: [], + explanations: [], shouldClose: false, shouldMerge: false, shouldUpdateLabels: true, @@ -78,7 +78,7 @@ function createEmptyActions(): Actions { return { labels: [], responseComments: [], - suggestions: [], + explanations: [], shouldClose: false, shouldMerge: false, shouldUpdateLabels: false, @@ -294,9 +294,9 @@ export function process(prInfo: BotResult, // 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 const headCommitAbbrOid = abbrOid(info.headCommitOid); @@ -433,9 +433,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`, @@ -465,15 +462,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(``, @@ -512,6 +500,9 @@ function createWelcomeComment(info: ExtendedPrInfo, post: (c: Comments.Comment) display(` * ${emoji(info.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`); @@ -532,7 +523,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})`); } diff --git a/src/execute-pr-actions.ts b/src/execute-pr-actions.ts index 70f36a53e..b60db3a1d 100644 --- a/src/execute-pr-actions.ts +++ b/src/execute-pr-actions.ts @@ -18,7 +18,7 @@ export async function executePrActions(actions: Actions, pr: PR_repository_pullR ...await getMutationsForProjectChanges(actions, pr), ...getMutationsForComments(actions, pr.id, botComments), ...getMutationsForCommentRemovals(actions, botComments), - ...getMutationsForSuggestions(actions, pr), + ...getMutationsForExplanations(actions, pr), ...getMutationsForChangingPRState(actions, pr), ]); if (!dry) { @@ -102,23 +102,28 @@ 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 }) => - createMutation("addPullRequestReviewThread", { + ...actions.explanations.map(({ path, startLine, endLine, body }) => endLine + ? createMutation("addPullRequestReviewThread", { pullRequestId: pr.id, path, startLine: startLine === endLine ? undefined : startLine, line: endLine, - body: "```suggestion\n" + text + "```", + body, + }) + : createMutation("addPullRequestReviewComment", { + pullRequestId: pr.id, + path, + body, }) ), createMutation("submitPullRequestReview", { 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 7b3bf0e54..1f41176f8 100644 --- a/src/pr-info.ts +++ b/src/pr-info.ts @@ -55,14 +55,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 expected 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 = { @@ -185,7 +184,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 @@ -217,8 +216,8 @@ export async function deriveStateForPR( const refs = { head: prInfo.headRefOid, 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( @@ -352,19 +351,19 @@ 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 checker = configSuspicious[basename]; - if (!checker) return { suspect: `edited` }; + if (!checker) return { body: `edited` }; const newText = await getContents("head"); // Removing tslint.json, tsconfig.json, package.json and // OTHER_FILES.txt is checked by the CI. Specifics are in my commit @@ -423,7 +422,7 @@ function makeChecker(expectedForm: any, expectedFormUrl: string, options?: { par if (options && "parse" in options) { suggestion = options.parse(newText); } else { - 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 newData = jsonDiff.deepClone(suggestion); if (options && "ignore" in options) options.ignore(newData); @@ -432,15 +431,12 @@ function makeChecker(expectedForm: any, expectedFormUrl: string, options?: { par // 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(newData, towardsIt)); - return { - suspect: vsMaster.suspect, - suggestion: makeSuggestion(), - }; + return makeSuggestion(); // Apply any preexisting diffs to towardsIt async function ignoreExistingDiffs(ref: keyof Refs) { @@ -490,7 +486,7 @@ function makeChecker(expectedForm: any, expectedFormUrl: string, options?: { par return { startLine, endLine, - text: suggestionLines.join(""), + body: vsMaster!.suspect + "\n```suggestion\n" + suggestionLines.join("") + "```", }; } };