Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue a PR to GH from extension #1

Merged
merged 5 commits into from May 24, 2019
Merged

Issue a PR to GH from extension #1

merged 5 commits into from May 24, 2019

Conversation

rvantonder
Copy link
Contributor

An extension for issuing a GitHub pull request on the client side (given a changeset and commit/PR metadata).

Doing this as a PR because I want feedback on TypeScript things :)

Exposes a top-level function issuePullRequestWithToken which kicks off the following sequence:

  • get the commit of the branch we will PR, or create it if it does not exist
  • create a git tree from the file(s) and content changes passed to the extension, and attach it to the PR branch
  • push the commit to the PR branch
  • create the pull request against a destination branch

All of the PR logic lives in pull-request-core.ts. I pulled this out of the extension due to sourcegraph/sourcegraph#733, so I could set up testing.

The extension entry point exposes the main command as pr.issuePullRequest.

Some simplifying assumptions for now:

  • One shot commit-and-PR, meaning
    • One commit attached to PR
    • No support to add multiple commits to an existing PR, or remove/close PR

See other comments inline.

src/pull-request-core.ts Outdated Show resolved Hide resolved
src/pull-request-core.ts Outdated Show resolved Hide resolved
@rvantonder rvantonder requested a review from sqs May 23, 2019 09:04
@rvantonder
Copy link
Contributor Author

cc @felixfbecker teach me things about TS :))

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated
@@ -0,0 +1,14 @@
{
"extends": "./node_modules/@sourcegraph/tsconfig/tsconfig.json",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"extends": "./node_modules/@sourcegraph/tsconfig/tsconfig.json",
"extends": "@sourcegraph/tsconfig",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I touched this file (I ran the Sourcegraph script to set up extensions). Unsure, but thanks for catching.

baseBranch: string,
commitBranch: string
): Promise<string> {
return new Promise((resolve, reject) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You almost never need the Promise constructor, unless you need to convert some other callback API to Promises. When you do need to use it, you should minimize the code inside it. Here you already have a Promise at hand so you can just return and chain that (or use async/await).

If you were looking for a way to reject the Promise, you just need to throw inside a handler or async function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Is throw new Error(...) a reasonable thing or are there better conventions for error handling?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new Error() is the way to go for errors.

@felixfbecker
Copy link

Here ya go :)

@rvantonder
Copy link
Contributor Author

That was really helpful thanks Felix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants