Skip to content

Sectors performance#32

Merged
rodolfobandeira merged 10 commits intodblock:masterfrom
gil27:fetch-sector-performance
Oct 5, 2018
Merged

Sectors performance#32
rodolfobandeira merged 10 commits intodblock:masterfrom
gil27:fetch-sector-performance

Conversation

@gil27
Copy link
Copy Markdown
Contributor

@gil27 gil27 commented Oct 2, 2018

Hello, guys. This PR address issue #31
I've tried to follow the code style, and be simple.

By the way, you guys have a good code base. 👍 👍 👍

The following code implements support to fetch sectors performance from
api.

It implements the feature inside lib/iex/api/sectors.

I'm looking forward your review

gil27 added 2 commits October 1, 2018 23:45
The following code implements support to fetch sectors peformance from
api.

It implements the feature inside `lib/iex/api/sectors`. For more
information check it out: #31
@dblock
Copy link
Copy Markdown
Owner

dblock commented Oct 2, 2018

Looks good. See bot comments above about the TOC and CHANGELOG.

Copy link
Copy Markdown
Collaborator

@rodolfobandeira rodolfobandeira left a comment

Choose a reason for hiding this comment

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

@gil27 Looks pretty good. Just added a few suggestions. Also. please add your name and description of your feature on CHANGELOG.md. Instruction here: https://github.com/dblock/iex-ruby-client/blob/master/CONTRIBUTING.md#update-changelog

Comment thread README.md Outdated
Fetches earnings for a symbol.

```ruby
earnings = IEX::Resources::Earnings.get('MSFT')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gil27 This should be: sectors = IEX::Resources::Sectors.get('MSFT')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shame on me 😭

Comment thread lib/iex/resources/sectors.rb
@gil27
Copy link
Copy Markdown
Contributor Author

gil27 commented Oct 3, 2018

Guys, I have been beat by danger here, could you guys help me out here to figure out what is the problem, I am missing something that I dont know what it is, for sure it is something dummy tough :/

@rodolfobandeira
Copy link
Copy Markdown
Collaborator

Hey @gil27

Please try this:

curl -o https://gist.githubusercontent.com/rodolfobandeira/cffea602a9727b6a9817f1d0195591a1/raw/5805ba093fd58b5a9db8e8febff8bcb718498cdf/fix-danger.patch

patch -p1 < fix-danger.patch

git commit -am 'Fix danger'; git push'

Comment thread README.md Outdated

### Get Sector Performance

Fetches earnings for a symbol.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Missed this one. It should be something like: Fetches latest sector's performance.

@dblock
Copy link
Copy Markdown
Owner

dblock commented Oct 3, 2018

Your missing a colon after the issue number in CHANGELOG:

* [#32](https://github.com/dblock/iex-ruby-client/pull/32): Add `IEX::Resource::Sectors` - [@gil27](https://github.com/gil27).

Copy link
Copy Markdown
Collaborator

@rodolfobandeira rodolfobandeira 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 to me!

@dblock What do you think?

@dblock
Copy link
Copy Markdown
Owner

dblock commented Oct 4, 2018

@rodolfobandeira you can merge at will, no need to ask me ;)

Comment thread CHANGELOG.md Outdated
### 0.5.0 (next)
* Your contribution here.

### 0.4.4 (2018/10/03)
Copy link
Copy Markdown
Owner

@dblock dblock Oct 4, 2018

Choose a reason for hiding this comment

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

We haven't released 0.4.4, please undo this change, it should just be 0.4.4 next.

Comment thread README.md Outdated
Fetches latest sector's performance.

```ruby
sectors = EX::Resources::Sectors.get('MARKET')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IEX instead of EX

@rodolfobandeira
Copy link
Copy Markdown
Collaborator

@gil27 Just one last small thing I found going through the last check.

sectors = EX::Resources::Sectors.get('MARKET') (Change it to IEX).

Once this goes in, we're good to merge!
Thanks again @gil27 !

@rodolfobandeira rodolfobandeira merged commit cf6a2f1 into dblock:master Oct 5, 2018
Comment thread CHANGELOG.md

### 0.4.3 (2018/08/18)

* Your contribution here.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That one has shipped, I'll edit in master.

@gil27 gil27 deleted the fetch-sector-performance branch October 10, 2018 00:54
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.

3 participants