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.

Sign up for GitHub

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

Jump to bottom

Adds ability to retrieve a company's logo #10

Merged
merged 1 commit into from
Jul 27, 2018
Merged

Adds ability to retrieve a company's logo #10

merged 1 commit into from
Jul 27, 2018

Conversation

rodolfobandeira
Copy link
Collaborator 

IEX::Resources::Logo.get('MSFT')
https://storage.googleapis.com/iex/api/logos/MSFT.png

@rodolfobandeira rodolfobandeira changed the title Adds hability to retrieve a company's logo Adds ability to retrieve a company's logo Jul 26, 2018
@dblock
Copy link
Owner

dblock commented Jul 26, 2018

Add a line to https://github.com/dblock/iex-ruby-client/blob/master/CHANGELOG.md and this is good to go! Thanks.

@rodolfobandeira
Copy link
Collaborator Author

@dblock Added. Thanks!

dblock
dblock reviewed Jul 26, 2018
View reviewed changes
spec/iex/resources/logo_spec.rb Outdated
end

it 'retrieves a logo' do
expect(subject['url']).to eq 'https://storage.googleapis.com/iex/api/logos/MSFT.png'
Copy link
Owner

Choose a reason for hiding this comment

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

I missed that it's returning a structure. This should actually return a 1st class object called Logo with a property called url. See how it's done for OHLC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I didn't notice that. So, now it should be good:

    9:     it 'retrieves a logo' do
   10:       require 'byebug'
   11:       byebug
=> 12:       expect(subject.url).to eq 'https://storage.googleapis.com/iex/api/logos/MSFT.png'
   13:     end
   14:   end
   15: end
(byebug) subject
#<IEX::Resources::Logo url="https://storage.googleapis.com/iex/api/logos/MSFT.png">
(byebug) subject.url
"https://storage.googleapis.com/iex/api/logos/MSFT.png"
(byebug)

dblock
dblock approved these changes Jul 27, 2018
View reviewed changes
Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks good, minor suggestions if you want to do them.

Needs to be rebased and green and ready to merge.

README.md Outdated
logo.url # 'https://storage.googleapis.com/iex/api/logos/MSFT.png'
```

See [#logo](https://iextrading.com/developer/docs/#logo) for detailed documentation or [logo.rb](lib/iex/resources/logo.rb) for returned field.
Copy link
Owner

Choose a reason for hiding this comment

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

"returned fields" (plural), even though single field right now probably.

spec/iex/resources/logo_spec.rb Outdated
require 'spec_helper'

describe IEX::Resources::Logo do
context "retrieve company's logo", vcr: { cassette_name: 'logo/msft' } do
Copy link
Owner

Choose a reason for hiding this comment

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

minor, "retrieves company's logo", just more idiomatic

@rodolfobandeira
Copy link
Collaborator Author

@dblock Fixed typos and rebase the latest master. Thanks!

dblock
dblock requested changes Jul 27, 2018
View reviewed changes
Copy link
Owner

@dblock dblock left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One minor issue in CHANGELOG I missed, then it's ready to merge.

Also a good practice for these pull requests is to squash them, so you end up with one single commit to avoid a squash on merge. Give it a try.

CHANGELOG.md Outdated
@@ -7,6 +7,7 @@
* [#5](https://github.com/dblock/iex-ruby-client/pull/5): Added `IEX::Resources::OHLC#get` - [@jromanovs](https://github.com/jromanovs).
* [#5](https://github.com/dblock/iex-ruby-client/pull/5): Added `IEX::Resources::OHLC#market` - [@jromanovs](https://github.com/jromanovs).
* [#9](https://github.com/dblock/iex-ruby-client/pull/9): Added `IEX::Errors::ClientError` - [@rodolfobandeira](https://github.com/rodolfobandeira).
* [#10](https://github.com/dblock/iex-ruby-client/pull/10): Added `IEX::Resources::Logo#get` - [@rodolfobandeira](https://github.com/rodolfobandeira).
Copy link
Owner

Choose a reason for hiding this comment

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

I missed this, this changelog entry and the one above belongs in 0.3.4 above, not in the released 0.3.3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Please let me know if that is in the correct format. I guess you'll add the new entry 0.3.5 and edit the date when you release the 0.3.4 right?

Thanks again!

Copy link
Owner

Choose a reason for hiding this comment

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

Right.

@dblock dblock merged commit 7fb1f1c into dblock:master Jul 27, 2018
@dblock
Copy link
Owner

dblock commented Jul 27, 2018

Merged, thanks!

@rodolfobandeira rodolfobandeira deleted the add-get-company-logo branch July 30, 2018 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rodolfobandeira @dblock