Small fixes & improvements

tyrauber / stock_quote

A ruby gem that retrieves stock quotes from IEX

MIT License

211 stars 55 forks source link

Small fixes & improvements #26

Closed alexbalonperin closed 8 years ago

alexbalonperin commented 8 years ago

Hello,

I have made the following changes:

adding .idea to .gitignore since I use IntelliJ for development
fix one failing test related to simple return
change the debugger dependency to byebug which works for newer version of ruby (2.x) since your gem works fine with ruby 2.x

Let me know if you are okay with those and if you prefer that I break the PR into smaller PRs.

There is another failing test which is related to some questionable behavior change on the server side. Basically, requests with symbol that don't exist are no longer returning 404 but 200 with nil values...

Anyway, I can also fix that spec if you tell me what behavior you want? Failure when values are nil or just do nothing?

Thank you

tyrauber commented 8 years ago

Well, that's unfortunate. I agree with your suggestion. I'd opt to do a failure when all values are nil, as it was originally intended. I don't think it should succeed if the symbol doesn't exists. Could potentially lead to bad data. Thanks for catching that. Would you mind making that change on this branch and then I'll merge it in and push a new build? Thanks for the Pull Request.

mattrayner commented 8 years ago

Any movement on this? I'd love to see these merged in 👍

tyrauber commented 8 years ago

@zolrag13, @mattrayner Merged and version bumped to 1.2.4.

alexbalonperin commented 8 years ago

@tyrauber @mattrayner I am sorry for getting back on this PR so late but things have been quite busy at work. I will make a new PR for the spec failure asap.

tyrauber commented 8 years ago

@zolrag13 No problem. I merged and resolved the spec failure in 1.2.4. Thanks for the contribution.

alexbalonperin commented 8 years ago

@tyrauber I would be happy to contribute further to your project. I was wondering if I could extend the library to provide additional finance related entities from the Yahoo API such as dividends, balance sheet, cash flow statement, ... or would you prefer to keep it focused on stock prices?

tyrauber commented 8 years ago

@zolrag13, I am not opposed. As long as the changes are backwards compatible, with adequate spec coverage, in the style and convention of the rest of the gem; I think I'd be all for it. I could see querying for stocks by dividend, assets or cash flow, to be really useful. Branch away!