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

Replace Github authorization endpoint by device authorization grant #496

Merged
merged 4 commits into from Nov 22, 2020

Conversation

rng-dynamics
Copy link
Collaborator

Fix issue #484

Suggestions for changes/improvements are very welcome.

Copy link
Owner

@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 :)

bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
@rng-dynamics
Copy link
Collaborator 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
Collaborator Author

In any case, further suggestions for improvements are welcome.

@rng-dynamics
Copy link
Collaborator 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
Owner

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

bridge/github/config.go Outdated Show resolved Hide resolved
bridge/github/config.go Outdated Show resolved Hide resolved
@MichaelMure
Copy link
Owner

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
Collaborator 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
Owner

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
Collaborator Author

Rebase completed @MichaelMure.

@MichaelMure MichaelMure merged commit 8d5c470 into MichaelMure:master Nov 22, 2020
@MichaelMure
Copy link
Owner

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.

None yet

2 participants