Adds custom Exceptions + more consistent spacing + documentation #49
Conversation
Most recent changes
|
Reviewing now! |
|
@Jamonek I realize my formatting changes (for consistency + readability) make this PR a bit difficult to review, but my main/impacting changes were with adding custom Exceptions for invalid ticker symbols |
Robinhood/Robinhood.py
Outdated
| import requests | ||
| import six | ||
|
|
||
| import logging |
There was a problem hiding this comment.
PEP import standard:
- stdlib libraries
- external dependencies
- internal imports
There was a problem hiding this comment.
Nice catch - just saw this here and fixed it.
I always thought it was external first so that the user/developer knew to install those first, followed by application-specific since that is next-most relevant, followed by stdlib since those were included by default.
Robinhood/Robinhood.py
Outdated
| class Bounds(Enum): | ||
| """enum for bounds in `historicals` endpoint""" | ||
| """ | ||
| Enum for bounds in `historicals` endpoint |
There was a problem hiding this comment.
adding newline for all docstrings is not required with Google/Napoleon style: http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html
There was a problem hiding this comment.
Good point - I just did it to increase readability. Would you like me to go back and fix all of it?
There was a problem hiding this comment.
Yes, b/c it will make a big mess when someone comes through with sphinx to make ReadTheDocs
Robinhood/Robinhood.py
Outdated
| def logout(self): | ||
| """logout from Robinhood | ||
| """ | ||
| Sogout from Robinhood |
There was a problem hiding this comment.
Fixed - made that typo when I was using Vim
Robinhood/Robinhood.py
Outdated
| self.endpoints['instruments'], | ||
| params={'query': stock.upper()} | ||
|
|
||
| res = self.session.get(self.endpoints['instruments'], |
There was a problem hiding this comment.
prefer old tabstop as per Kevlin Henney: https://www.youtube.com/watch?v=ZsHMHukIlJY
Robinhood/Robinhood.py
Outdated
| #Check for validity of symbol | ||
| try: | ||
| req = requests.get(url) | ||
| req.raise_for_status() | ||
| data = req.json() | ||
|
|
There was a problem hiding this comment.
extra whitespace between try/except?
There was a problem hiding this comment.
Fixed --> I added it for additional readability, but I also know it's not part of the style-guide
Robinhood/Robinhood.py
Outdated
| except requests.exceptions.HTTPError: | ||
| raise NameError('Invalid Symbols: ' + ",".join(stocks)) #TODO: custom exception | ||
| raise RH_exception.InvalidTickerSymbol() |
There was a problem hiding this comment.
Yay! Finally your stamp of approval :P
Robinhood/Robinhood.py
Outdated
| interval = day + span = year | ||
| interval = week | ||
| TODO: NEEDS TESTS | ||
| def get_historical_quotes(self, stock, interval, span, bounds=Bounds.REGULAR): |
There was a problem hiding this comment.
>2 args are split up b/c it's easier to add/remove/comment args in debug
There was a problem hiding this comment.
Debatable because the line length is still under 100 characters and it's fairly easy to read/see the parameters and default parameters.
If we had more parameters or parameters with default variables or more verbose names, I would split them up.
There was a problem hiding this comment.
consistency is king. It's an easy rule to follow, and nearly every other >2 arg function looks one way.
Robinhood/Robinhood.py
Outdated
|
|
||
| """ | ||
|
|
||
| #Will be in format: 'YYYY-MM-ddTHH:mm:ss:000Z' |
There was a problem hiding this comment.
move to Notes section in docstring instead?
There was a problem hiding this comment.
Good point - added
Robinhood/Robinhood.py
Outdated
| def securities_owned(self): | ||
| """ | ||
| Returns a list of symbols of securities of which there are more | ||
| than zero shares in user's portfolio. | ||
| Returns a list of symbols of securities of which there are more |
There was a problem hiding this comment.
80 char warning, 100 char line break. This might be too restrictive?
There was a problem hiding this comment.
Grammar change --> might be more readable/concise
|
General notes:
|
|
Thank you for all the feedback @lockefox - I really appreciate it from a learning standpoint, and I love how attentive you are to all the details. I implemented your changes/feedback, so please let me know what you think |
Robinhood/Robinhood.py
Outdated
| """ | ||
| Robinhood.py: a collection of utilities for working with Robinhood's Private API | ||
| """ | ||
| """ Robinhood.py: a collection of utilities for working with Robinhood's Private API """ |
There was a problem hiding this comment.
just a note: you can do multi-line headers with RST if you want special notes
|
@dsouzarc your docstrings still have a leading space between |
|
@lockefox Okay, we should be good to go now. I didn't notice the lack of leading space as a requirement |
|
👍 |
|
Great work ! |
References: #48
#47