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

merge http params for application auth #338

Conversation

rmetzler
Copy link
Contributor

This is a bugfix for issue #337

There are no tests, but the ones in the referenced issue. Please advice on how and where to add tests.

Thanks.

@pengwynn
Copy link
Collaborator

D'oh duplicating effort. What do you think about #339 instead?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c697eea on rmetzler:feature/dont-forget-application-authentication-bug-337 into 4d3ac13 on octokit:master.

@rmetzler
Copy link
Contributor Author

I like your tests. Also root is simplified.
I'm a sad 🐼 I won't become a Octokit contributor.

@rmetzler
Copy link
Contributor Author

looking in the code again it seems we both fix it on different layers. Maybe we should merge both pull requests?

pengwynn added a commit that referenced this pull request Nov 14, 2013
…-authentication-bug-337

merge http params for application auth
@pengwynn pengwynn merged commit 2025c4a into octokit:master Nov 14, 2013
@pengwynn
Copy link
Collaborator

You twisted my arm? 😄

Thanks for the patch.

@rmetzler
Copy link
Contributor Author

thanks again @pengwynn GOOD JOB! 👯

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.

3 participants