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

Backfill breaking change #89

Merged
merged 4 commits into from
Nov 1, 2020
Merged

Conversation

agrberg
Copy link
Collaborator

@agrberg agrberg commented Oct 31, 2020

I found a breaking change where we could no longer assign the logger through the config object. The following was broken by #88

IEX::Api.configure do |config|
  config.logger = logger
end
# and
IEX::Api::Client.new(logger: logger)

I fixed that in this PR as to not release a Gem version w/ a breaking change unless necessary. I also changed how IEX::Api is augmented to provide logger and config. I believe it makes it clearer where the methods come from as well as making those in Logger reusable for Config::Client's extension. Let me know what you think.

Nest existing test for coming compatibility test
Test assumed behavior
Fix Rubocop violation around `extend self` in `Module`s.
  `class << self` is clearest in this case because we need to set attr_accessors on the Module as well as the `reset!` method.
  `module_function` also pollutes the including object as it gets a private method of the same name.
  No need to include instance methods on Config::Client or include it in Api::Client if the ATTRIBUTE array is simply splatted into attr_accessor (and is easier to follow where it comes from)
@agrberg agrberg requested a review from dblock October 31, 2020 22:12
Extract Config accessory to module for clearer inclusion/extension into IEX::Api
@agrberg agrberg force-pushed the backfill_breaking_change branch from 12562b8 to 46365db Compare October 31, 2020 22:30
@@ -445,7 +445,7 @@ client.post('ref-data/isin', isin: ['US0378331005'], token: 'secret_token') # [{
You can configure client options globally or directly with a `IEX::Api::Client` instance.

```ruby
IEX::Api::Client.configure do |config|
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't dig too deeply but going back to 1.2.0 before I started screwin' around there was no IEX::Api::Client.configure method. I believe this was a mistake in the docs between IEX::Api.configure and IEX::Api::Client.new(…)

@dblock dblock merged commit f852029 into dblock:master Nov 1, 2020
@agrberg agrberg deleted the backfill_breaking_change branch November 2, 2020 01:01
@agrberg agrberg restored the backfill_breaking_change branch November 2, 2020 01:06
@agrberg agrberg deleted the backfill_breaking_change branch November 2, 2020 01:07
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