coderly / code

The Coderly toolbelt
http://coderly.com
1 stars 0 forks source link

Implement "code pr" opening an existing pull request if one exists #9

Closed begedin closed 9 years ago

begedin commented 9 years ago

Description

Branching off of PR #8 to avoid potential trouble and to have something to make a PR out of (for testing). Nothing is implemented yet. In hindsight, this wasn't actually necessary.

The actuall changes are in

Unfortunately, there is no simple way to find a PR from the command-line or via hub. The only way this seems to be possible is through the GitHub API.

I actually contacted github and was promptly answered that this is exactly the case:

Yep, that's possible, but you need to use the API:

https://developer.github.com/v3/pulls/#list-pull-requests

Notice that you can specify the head and/or the base branch for that API endpoint in the query parameters, which allows you to fetch the list of pull requests for those branches.

E.g. here's the pull request for the push-changes branch in github/linguist

https://api.github.com/repos/github/linguist/pulls?head=github:push-changes

Does that help?

Since that would mean we'd have to handle authentication on code side instead of relying on hub authentication, I found a gem called github_api as well as one called octokit.

After some testing, I decided to go with the second one. It seems more developed and more robust than the first.

Issues

Unfortunately, I have difficulties with code due to using linux, which I haven't realized I had before.

Basically, code has issues on linux :)

Flow

To elaborate, with this feature, the user is required to log in once, using

code authorize --username "username" --password "password"

After that an oauth token will be created and stored into the global .gitconfig file. The token will be used to fetch and the proper pull request url and open it in browser.

I don't see a way around this, but I'm thinking there should also be a prompt in that case, to ask for credentials if code pr was called before code authorize being called first. Either that, or we make code pr accept a --username and --password option to and call authorize internally if it's needed and the options are provided, displaying an error otherwise.

Basically

NOTE: I decided to go with Solution A for the time being. It's faster to implement and more typical of command line behavior.

begedin commented 9 years ago

@venkatd: I did some refactoring which was made possible by switching from using System.call to System.result (we don't want the configuration retrieval calls to get printed out to the standard output anyway, in my opinion).

I decided not to switch to the additional layer of a config object, for two simple reasons:

If we end up having more in the future, then we'd have a case for adding a different layer, but for now, it seems like it would add complexity - mostly due to the special error case.

venkatd commented 9 years ago

@begedin do you prefer solution A or solution B? If you present several solutions, it will be a good habit to explain which option you prefer and why.

begedin commented 9 years ago

Sorry, I meant to add a comment. I went with solution A because it seems more straightforward and typical of command line behavior. The user is explained they need to call "code authorize" once, but isn't prompted for credentials directly.