-
-
Notifications
You must be signed in to change notification settings - Fork 216
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
Option to store scopes from response header on client #382
base: master
Are you sure you want to change the base?
Conversation
I intentionally did not add the option as a request-level parameter, because it would only work on the first request made by each client instance, given that |
Generated by 🚫 Danger |
@@ -6,7 +6,7 @@ class Client | |||
include Faraday::Request | |||
include Api::Endpoints | |||
|
|||
attr_accessor(*Config::ATTRIBUTES) | |||
attr_accessor(*Config::ATTRIBUTES, :oauth_scopes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about making this attr_reader :oauth_scopes
so that it wouldn't appear that you can set the scopes here. But then it would need to use instance_variable_set
from the middleware to set them. Not sure which is better.
@@ -26,9 +26,9 @@ Metrics/AbcSize: | |||
# Offense count: 2 | |||
# Configuration parameters: IgnoredMethods. | |||
Metrics/CyclomaticComplexity: | |||
Max: 9 | |||
Max: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule and PerceivedComplexity
were failing on connection
with the newly added middleware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I just run rubocop -a ; rubocop --auto-gen-config
.
This is good, but I think we can do better. What do you think of being able to write this: config.connection do |connection, options|
connection.use ::Slack::Web::Faraday::Response::StoreScopes, options
end This way clients can install any middleware. Another option could be adding config.middleware << ::Slack::Web::Faraday::Response::StoreScopes Not sure which one I prefer, would take either. |
@dblock How would your first option work exactly? Would the block be stored in the config as a proc, and then called with the connection instance during the In both cases how would it handle the fact that I'm passing the instance of the client to Also with these options would the user be able to set this at the client instance level, or only at the config level? In my case, since I only need to use this in a specific place with Slack::Web::Client.new(token: token, store_scopes: true).auth_test |
Right, you would
I think you could hide this away. I didn't write the code but I would want the supporting config code to always pass in
It should work in both. If you are having trouble implementing this LMK and I could give it a shot. I don't know whether it's possible - I am only thinking about what the user would want to write, and leaving the implementation to the professionals ;) |
1963b52
to
097b3ca
Compare
Resolves #378
This adds an optional response middleware to the Web client. When used the middleware pulls the OAuth scopes from the response header
x-oauth-scopes
and puts them on the instance of the client atclient.oauth_scopes
.I wasn't sure the best way to gain access to the client itself from the middleware, so I followed the pattern used by the
Mashify
middleware to pass in an option. Maybe there's a better way?I'm also not sure about the naming of the middleware class (
StoreScopes
) or the config option (store_scopes
).No tests or documentation yet.