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

Extract configurations into separate object #939

Merged

Conversation

joeltaylor
Copy link
Contributor

Adds a Stripe::StripeConfiguration object to manage internal and user supplied configuration options.

This is primarily motivated by discussion in #921 in order to provide a way to set options for an instance of StripeClient.

lib/stripe.rb Outdated
Comment on lines 58 to 85
# User configurable options
def_delegators :@configuration, :api_key, :api_key=
def_delegators :@configuration, :api_version, :api_version=
def_delegators :@configuration, :stripe_account, :stripe_account=
def_delegators :@configuration, :api_base, :api_base=
def_delegators :@configuration, :uploads_base, :uploads_base=
def_delegators :@configuration, :connect_base, :connect_base=
def_delegators :@configuration, :open_timeout, :open_timeout=
def_delegators :@configuration, :read_timeout, :read_timeout=
def_delegators :@configuration, :proxy, :proxy=
def_delegators :@configuration, :verify_ssl_certs, :verify_ssl_certs=
def_delegators :@configuration, :ca_bundle_path, :ca_bundle_path=
def_delegators :@configuration, :log_level, :log_level=
def_delegators :@configuration, :logger, :logger=
def_delegators :@configuration, :max_network_retries, :max_network_retries=
def_delegators :@configuration, :enable_telemetry=, :enable_telemetry?

# Internal configuarations
def_delegators :@configuration, :max_network_retry_delay
def_delegators :@configuration, :initial_network_retry_delay
def_delegators :@configuration, :ca_store
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this blob is too large, I can always break them down into relevant groupings like they were previously.

def reverse_duplicate_merge(hash)
dup.tap do |instance|
hash.each do |option, value|
instance.public_send("#{option}=", value)
end
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I intend to use in #921 for the client configuration.

I went back and forth on whether or not the duplicate configuration object should be a snapshot or a pointer with overrides (e.g. any options not set would continually point to the global config object in the event those change). However, I feel like that could lead to unexpected behavior, which is why I favored a snapshot.

assert_equal "path/to/ca/bundle", Stripe.ca_bundle_path
ensure
Stripe.ca_bundle_path = old
context "forwardable configurations" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These specs are slightly redundant, but they're cheap and lock in the expected API on Stripe for configuration.

@read_timeout = 80

@enable_telemetry = true
@configuration = Stripe::StripeConfiguration.setup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably expose this if there's ever a desire to permit something like:

Stripe.configure do |config|
  # config.read_timeout = 100
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Let's expose it separately if we decided to do so, but I think this configuration style would be much more "Ruby-ish" than what the gem currently offers.

@joeltaylor joeltaylor force-pushed the extract-configuration-into-object branch from 518b82b to 3b08dfa Compare August 9, 2020 22:33
Comment on lines +53 to +56
# map to the same values as the standard library's logger
LEVEL_DEBUG = Logger::DEBUG
LEVEL_ERROR = Logger::ERROR
LEVEL_INFO = Logger::INFO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left the CA and LEVEL constants here in case folks reference them from outside the gem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call.

@joeltaylor joeltaylor force-pushed the extract-configuration-into-object branch 3 times, most recently from 73bb2c7 to 6924f85 Compare August 9, 2020 23:06
@joeltaylor
Copy link
Contributor Author

@brandur-stripe wasn't sure what you envisioned the configuration options looking like for #921, but figured I'd take a stab at refactoring it. This should make it fairly easy to pass a configuration object around while defaulting to the global configuration. Happy to explore other routes as well if this doesn't sit well.

lib/stripe.rb Outdated
def_delegators :@configuration, :max_network_retries, :max_network_retries=
def_delegators :@configuration, :enable_telemetry=, :enable_telemetry?

# Internal configuarations
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny typo here: "configurations"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! I really need to configure my editor to spell check comments 😅

@brandur-stripe
Copy link
Contributor

@joeltaylor Wow, there's been a very concerning pattern on my side where you pitch awesome PRs over and it takes me forever to get back to you. Sorry about that!

Found a minor typo, but this looks great to me. I'm going to solicit feedback internally in case there is any, but will time out in a day or so. This looks good to merge.

@joeltaylor
Copy link
Contributor Author

@brandur-stripe No worries at all – I appreciate you taking the time to review the PR! Once it's all squared away I can plug it into the other PR.

Adds a `Stripe::StripeConfiguration` object to manage internal and user
supplied configuration options.

This is primarily motivated by stripe#921 in order to provide a way to set
options on for an instance of `StripeClient`.
@joeltaylor joeltaylor force-pushed the extract-configuration-into-object branch from 6924f85 to 174935e Compare August 25, 2020 21:52
@brandur-stripe
Copy link
Contributor

That would be amazing Joel :) I had a colleague look at this as well, and then liked it too. Thanks again!

@brandur-stripe brandur-stripe merged commit 23a0ee2 into stripe:master Aug 26, 2020
@brandur-stripe
Copy link
Contributor

Pushed as 5.24.0.

@joeltaylor
Copy link
Contributor Author

Thanks @brandur-stripe! I'll ping you once I integrate this into the other PR.

@brandur-stripe
Copy link
Contributor

Roger that! TY.

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