search

ConradSelig / cliStocksTracker

A command line utility for tracking a stock market portfolio. Primarily featuring high resolution braille graphs.
Apache License 2.0
56 stars 14 forks source link

Performance improvements #30

Closed patrick-hovsepian closed 3 years ago

patrick-hovsepian commented 3 years ago

@ConradSelig This builds off of my previous PR and I am just seeing your comments there.

This PR is mainly for performance improvements - the biggest change is fetching all ticker data in one request rather than a request per ticker.

If you're not opposed, I'll work the feedback from the previous PR into this one and just close that.

I'm aware of the merge conflicts and will address those - in the meantime, wanted to get your eyes on this.

ConradSelig commented 3 years ago

Overall looks great so far - I spent about 5 minutes initially working with yfinance to get what I needed, never even bothered to look back to see if there was a better way to do it.

Also thought of another performance improvement we could make... see #31, if you want to implement that here awesome! Otherwise don't sweat it, we can catch it later.

Can't wait to deep-dive into the final pull request!

ConradSelig commented 3 years ago

@patrick-hovsepian Let me know when you're happy with the pull request, I'm already doing some testing but don't want to accept it before you are done making changes.

patrick-hovsepian commented 3 years ago

@ConradSelig should be ready for final approval