Remove Octokit::Client::Authorizations #1494
Conversation
| # @param options [Hash] Header params for request | ||
| # @return [Array<String>] OAuth scopes | ||
| # @see https://developer.github.com/v3/oauth/#scopes | ||
| def scopes(token = @access_token, options = {}) |
There was a problem hiding this comment.
It might be worth keeping this one? 🤔 It isn't specifically related to the Authorizations API and it could be useful for users. I'm not sure we have any replacement to it either, as I don't think there is a way to access headers in the SDK.
There was a problem hiding this comment.
Good point! Would you rather I keep the authorizations module around for this use case, or refactor this functionality somewhere else?
There was a problem hiding this comment.
Kinda up to you, honestly. I would probably move it ("authorizations" isn't really a sensible name, since I think it works for any token?).
kfcampbell
force-pushed
the
remove-octokit-client-authorizations
branch
from
5658816
to
2e251af
Compare
|
@timrogers Apologies for the force-push! It took me a minute to understand that vcr was reaching out to an actual endpoint and auto-generating failing cassettes when I first renamed the Authorizations module. Is this fix something like you had in mind? I've gone for |
lib/octokit/client/tokens.rb
Outdated
| # @example Create a new OR return an existing authorization to be used by a specific client for user ctshryock's project Zoidberg | ||
| # client = Octokit::Client.new(:login => 'ctshryock', :password => 'secret') | ||
| # client.create_authorization({:idempotent => true, :client_id => 'xxxx', :client_secret => 'yyyy', :scopes => ["user"]}) | ||
| def create_authorization(options = {}) |
There was a problem hiding this comment.
Why is this required to test #scopes? That method looks like it should work with any token and get its permissions.
There was a problem hiding this comment.
In the test here an authorization is required to be created before checking its scopes.
Optionally, we could remove that method and also remove the testing for scopes, which doesn't feel ideal. I'm not seeing an easy way to keep a valid test of scopes without that method. Thoughts?
There was a problem hiding this comment.
Could we not just call #scopes directly with a token, rather than creating one with #create_authorization?
There was a problem hiding this comment.
Welllllll of course we could do that! Forgive my slowness, and please take another look!
|
I'm unsure of the proper Ruby practice around cassettes or how to autogenerate/remove them. The create_authorizations cassettes are now safe to remove so I've done so manually. |
timrogers
force-pushed
the
remove-octokit-client-authorizations
branch
from
918c928
to
8850326
Compare
|
@timrogers Do you think this is okay to merge after your update? |
nickfloyd
added
Type: Maintenance
Fixes #1429.
This PR should land in a breaking change as Tim noted in the above issue, even if the underlying APIs are no longer functional.