Merge pull request #50 from Financial-Times/pulls-merge

refactor: require a pull request url for the `pulls merge` command

Author: Samuel Parkinson

lib/common-yargs.js

@@ -75,6 +75,21 @@ const withTitle = yargs => yargs.option("title", {
 	type: "string",
 })
 
+const withPullRequest = yargs => yargs.positional("pull-request", {
+	describe: "Pull request URL, e.g. https://github.com/foo/bar/pull/3",
+	type: "string",
+	coerce: pullRequest => {
+		const re = /https:\/\/github\.com\/(?<owner>.+)\/(?<repo>.+)\/pull\/(?<number>\d+)/;
+		const matches = re.exec(pullRequest);
+
+		if (matches === null) {
+			throw new Error(`"${pullRequest}" is not a valid pull request URL`);
+		}
+
+		return matches.groups;
+	}
+});
+
 module.exports = {
 	descriptions,
 	withToken,
@@ -87,4 +102,5 @@ module.exports = {
 	withTeamReviewers,
 	withBody,
 	withTitle,
+	withPullRequest,
 }

src/commands/pulls/merge.js

@@ -17,77 +17,42 @@ const builder = yargs => {
 	const baseOptions = flow([
 		commonYargs.withToken,
 		commonYargs.withJson,
-		commonYargs.withBase,
-		commonYargs.withOwner,
-		commonYargs.withRepo,
-		commonYargs.withNumber,
-	])
+		commonYargs.withPullRequest,
+	]);
+
 	return baseOptions(yargs)
-		.option("commit_title", {
-			alias: "t",
-			describe: "Pull request commit title",
-			type: "string"
-		})
-		.option("commit_message", {
-			alias: "m",
-			describe: "Pull request commit message",
-			type: "string"
-		})
-		.option("sha", {
-			describe: "SHA that the pull request head must match to allow merge",
-			type: "string"
-		})
-		.option("merge_method", {
-			describe: "Merge method to use. Possible values are merge, squash or rebase. Default is merge.",
-			type: "string"
-		})
+		.option("method", {
+			describe: "Merge method to use.",
+			choices: ["merge", "squash", "rebase"],
+			default: "merge"
+		});
 }
 
 /**
  * Return the contents of a pull request body and create a pull request.
  *
  * @param {object} argv - argv parsed and filtered by yargs
- * @param {string} argv.token
- * @param {string} argv.json
- * @param {string} argv.owner
- * @param {string} argv.repo
- * @param {string} argv.number
- * @param {string} [argv.commit_title]
- * @param {string} [argv.commit_message]
- * @param {string} [argv.sha]
- * @param {string} [argv.merge_method]
  * @throws {Error} - Throws an error if any required properties are invalid
  */
-const handler = async ({ token, json, owner, repo, number, commit_title, commit_message, sha, merge_method }) => {
+const handler = async (argv) => {
+	try {
+		const octokit = await authenticatedOctokit({ personalAccessToken: argv.token });
 
-	// Ensure that all required properties have values
-	const requiredProperties = {
-		owner,
-		repo,
-		number,
-	}
-	if (Object.values(requiredProperties).some(property => !property)) {
-		throw new Error(`Please provide all required properties: ${Object.keys(requiredProperties).join(", ")}`)
-	}
+		const result = await octokit.pulls.merge({
+			owner: argv.pullRequest.owner,
+			repo: argv.pullRequest.repo,
+			pull_number: argv.pullRequest.number,
+			merge_method: argv.method,
+		});
 
-	const inputs = Object.assign({}, requiredProperties, {
-		commit_title,
-		commit_message,
-		sha,
-		merge_method,
-	})
-	try {
-		const octokit = await authenticatedOctokit({ personalAccessToken: token })
-		const result = await octokit.pulls.merge(inputs)
-		printOutput({ json, resource: result })
-	}
-	catch (error) {
-		throw new Error(error)
+		printOutput({ json: argv.json, resource: result });
+	} catch (error) {
+		throw new Error(error);
 	}
 }
 
 module.exports = {
-	command: "merge",
+	command: "merge <pull-request>",
 	desc: "Merge an existing pull request",
 	builder,
 	handler

test/commands/pulls/merge.test.js

@@ -34,11 +34,22 @@ describe("Yargs", () => {
 		expect(console.warn).not.toBeCalled()
 	})
 
+	test("an invalid pull request URL errors", async () => {
+		expect.assertions(1)
+		try {
+			const testOptions = {
+				token: "test",
+				pullRequest: "foo-bar",
+			};
+			await yargsModule.handler(testOptions)
+		} catch (error) {
+			expect(error).toBeInstanceOf(Error)
+		}
+	})
+
 	const requiredOptions = {
 		token: "test",
-		owner: "test",
-		repo: "test",
-		number: "test",
+		pullRequest: "https://github.com/test/test/",
 	}
 	for (let option of Object.keys(requiredOptions)) {
 		test(`Running the command handler without '${option}' throws an error`, async () => {
@@ -55,7 +66,6 @@ describe("Yargs", () => {
 })
 
 describe("Octokit", () => {
-
 	// If this endpoint is not called, nock.isDone() will be false.
 	nock('https://api.github.com')
 		.persist()
@@ -65,9 +75,11 @@ describe("Octokit", () => {
 	test("running the command handler triggers a network request of the GitHub API", async () => {
 		await yargsModule.handler({
 			token: "test",
-			owner: "test",
-			repo: "test",
-			number: 1,
+			pullRequest: {
+				owner: "test",
+				repo: "test",
+				number: 1
+			},
 		})
 		expect(nock.isDone()).toBe(true)
 	})