Skip to content

Comments

Replace Github authorization endpoint by device authorization grant#496

Merged
MichaelMure merged 4 commits intogit-bug:masterfrom
rng-dynamics:master
Nov 22, 2020
Merged

Replace Github authorization endpoint by device authorization grant#496
MichaelMure merged 4 commits intogit-bug:masterfrom
rng-dynamics:master

Conversation

@rng-dynamics
Copy link
Contributor

Fix issue #484

Suggestions for changes/improvements are very welcome.

Copy link
Contributor

@MichaelMure MichaelMure left a comment

Choose a reason for hiding this comment

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

I nitpicked a lot but it's really superficial. Thanks :)

@rng-dynamics
Copy link
Contributor Author

I am not really sure why the tests in Travis CI are failing. Is it my error or is it a configuration problem?

@rng-dynamics
Copy link
Contributor Author

In any case, further suggestions for improvements are welcome.

@rng-dynamics
Copy link
Contributor Author

I also would like to ask you about your opinion of the new implementation of
func requestUserVerificationCode(scope string) (map[string]string, error).
It returns a map[string]string which holds 4 values originating from the response of the Github API. I would like to use all the values but do not know what would be a clean way to return all of them.

@MichaelMure
Copy link
Contributor

The tests fail because of:

bridge/github/config.go:219:25: Errorf call has arguments but no formatting directives
bridge/github/config.go:260:25: Errorf call has arguments but no formatting directives

Easy to fix

@MichaelMure
Copy link
Contributor

I also would like to ask you about your opinion of the new implementation of
func requestUserVerificationCode(scope string) (map[string]string, error).
It returns a map[string]string which holds 4 values originating from the response of the Github API. I would like to use all the values but do not know what would be a clean way to return all of them.

Returning a map is not great indeed. You can either make up your own struct with 4 fields to return those values or return 4 values + an error as go allow you to.

@rng-dynamics rng-dynamics marked this pull request as ready for review November 17, 2020 19:13
@rng-dynamics
Copy link
Contributor Author

I have a question since I am not very familiar with Github: Can I rebase my branch now onto your current master and force-push into my fork? I am wondering if it would create problems in this pull request.

@MichaelMure
Copy link
Contributor

Rebase and push --force is the correct way. It sometimes get github a bit confused if it can't reattach the comments at the correct place but that's not a bit deal. Just don't do it to often. Rebasing to have the updates from master is a very valid reason to do that.

@rng-dynamics
Copy link
Contributor Author

Rebase completed @MichaelMure.

@MichaelMure MichaelMure merged commit 8d5c470 into git-bug:master Nov 22, 2020
@MichaelMure
Copy link
Contributor

Well, that's awesome, thank you!
FYI, I cleaned it up just a little bit with 9daa8ad, notably the defer in the loop (defer trigger when the function return, not after each loop, which means the bodies were accumulating in memory).

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.

2 participants