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.

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

Add option base model #224

Open
wants to merge 68 commits into
base: master
Choose a base branch
from

Conversation

jeffweng8
Copy link
Contributor

@jeffweng8 jeffweng8 commented Apr 14, 2020

Checklist

  • Added tests for changed code.
  • Updated documentation for changed code.
  • I've added a news fragment of my changes with the name,
    "{ISSUE_NUM}.{feature|bugfix|doc|removal|misc}""

Related Issue

#201

Description

  • Updated several of the endpoints to be able to support options
  • Streamline instrument getter functions
  • Add option model class

pyrh/robinhood.py Outdated Show resolved Hide resolved
-make its own endpoint using instruments endpoint
pyrh/robinhood.py Outdated Show resolved Hide resolved
pyrh/robinhood.py Outdated Show resolved Hide resolved
pyrh/models/option.py Outdated Show resolved Hide resolved
pyrh/models/option.py Outdated Show resolved Hide resolved
@adithyabsk
Copy link
Member

Just ping me when you want me to take a look again. Also, if you're having trouble with the linter, take a look at the docs to see how to set up pre-commit which should automatically fix most of the issues.

@jeffweng8
Copy link
Contributor Author

Hey @adithyabsk

I am getting
pyrh/common.py:5: error: Cannot find implementation or library stub for module named 'yarl' pyrh/common.py:5: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports pyrh/models/base.py:6: error: Cannot find implementation or library stub for module named 'marshmallow' pyrh/models/base.py:78: error: Class cannot subclass 'Schema' (has type 'Any') pyrh/models/portfolio.py:5: error: Cannot find implementation or library stub for module named 'marshmallow' pyrh/models/option.py:5: error: Cannot find implementation or library stub for module named 'marshmallow' pyrh/models/oauth.py:6: error: Cannot find implementation or library stub for module named 'marshmallow' pyrh/models/oauth.py:46: error: unused 'type: ignore' comment pyrh/models/sessionmanager.py:11: error: Cannot find implementation or library stub for module named 'marshmallow' pyrh/models/sessionmanager.py:14: error: Cannot find implementation or library stub for module named 'yarl' pyrh/models/sessionmanager.py:497: error: unused 'type: ignore' comment Found 10 errors in 6 files (checked 13 source files)
when I try to run the linter locally. Do you know how to resolve this issue?

@adithyabsk
Copy link
Member

@jeffweng8 I think you might not have the packages installed locally, perhaps? (or at least not in a place mypy can find them) pre-commit is installed in a separate virtualenv so that it doesn't depend on your env but mypy needs those installations to properly static check our code. Can you try manually running mypy pyrh from the project base directory?

@jeffweng8
Copy link
Contributor Author

Ah that makes sense, I just installed yarl and marshmallow in the virtual environment and that fixed the issue. Thanks!

@adithyabsk
Copy link
Member

@jeffweng8 you likely want to install all of our dev dependencies either using pip install . or using poetry install -vvv (poetry is reccomended)

@jeffweng8
Copy link
Contributor Author

jeffweng8 commented Apr 15, 2020

Hmm I am getting this error when I try to run the poetry install command:

Traceback (most recent call last):
File "/Users/jeff/anaconda3/bin/poetry", line 7, in
from poetry.console import main
ModuleNotFoundError: No module named 'poetry'

Any idea where that is coming from? I ran pip install . and it successfully ran.

Also, when I try to run pytest, I get the following:
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --xdoc --cov=pyrh --cov-config=setup.cfg --cov-report=term --cov-report=html
inifile: /Users/jeff/Documents/pyrh/setup.cfg
rootdir: /Users/jeff/Documents/pyrh

Is this related to the above issue?

@jeffweng8
Copy link
Contributor Author

Also should I close this PR and make a new PR where I squash my commits?

@adithyabsk
Copy link
Member

Nope, no worries. I can squash when I merge @jeffweng8

@adithyabsk
Copy link
Member

Yep @jeffweng8, so I would actually suggest you not use your anaconda version of python when installing poetry since anaconda comes with a bunch of packages pre-installed.

$ which python

Should tell you the info you need there. I would use your system version of python (pre-installed on macs or brew version of python or better yet use pyenv to manage your python version)

You do this by looking at your PATH, which you likely modified to be able to use anaconda (putting it first in front of other directories your system would want to search)

@adithyabsk
Copy link
Member

Additionally if you use poetry it will automatically manage your virtual environment so you can just do poetry run [CMD] and access you venv easily. (E.g. poetry run pytest

@adithyabsk
Copy link
Member

adithyabsk commented Apr 15, 2020

It is also important to note how you install poetry, you shouldn’t install it just using pip, take a look at their docs and there should be a one line command that your run which uses your defined python version in your path to set it up.

@adithyabsk
Copy link
Member

@jeffweng8 just to keep you in the loop, I'm starting to work on re-factoring the endpoints part of the project. That's the next big task. I've started the a draft PR #226. Take a look to get a sense of the direction that I'm heading there.

@jeffweng8
Copy link
Contributor Author

Hey @adithyabsk

I just pushed the skeleton for the chain model. I need to fix the session manager for get_chain, but I'm not too sure what exactly that is supposed to be

@adithyabsk
Copy link
Member

@jeffweng8 I'll be quite busy for the next few weeks but I'll try to take a look this weekend. Also let me know when this PR is in a good state for CR and I'll try to get that in.

Also, to give you a sense of my next steps, I know I need to step up a better way to run tests on this project especially with Robinhood constantly updating their API. To that end, I'm likely going to try and set up vcr.py and pytest-vcr so that writing tests is as easy as calling the desired endpoints and caching the results. It will be somewhat difficult to initially set it up to work with oauth but once that is done it should be smooth sailing from there.

@adithyabsk
Copy link
Member

I need to fix the session manager for get_chain

Also, what specifically is broken?

@jeffweng8
Copy link
Contributor Author

jeffweng8 commented Apr 21, 2020

No rush, I was just unsure what exactly a session manager is supposed to be. Is it just getting the results from a page? If so should I create a default one since most of them should just be grabbing it from the "results" key?

@adithyabsk
Copy link
Member

SessionManager allows you to authenticate with the Robinhood API. Take a look at how I structured the Portfolio modules. I made a PortfolioManager which the main Robinhood class inherits from. The idea is that all "managers" should inherit from it and as a result get access to the managed forms of the get and post methods which allow you to hit the endpoints that are restricted to only logged in users.

Eventually, we should be able to mark specific methods as not requiring log-in which should work without the authentication flow.

@jeffweng8
Copy link
Contributor Author

jeffweng8 commented Apr 26, 2020

Hi @adithyabsk, I think this PR is pretty much ready. Could you help me see why this is failing some of the checks? When I run pytest locally, everything passes, but it appears that this is happening for some of the checks here:

Creating virtualenv pyrh-6aazBjZI-py3.7 in /Users/runner/.virtualenvs [FileNotFoundError] [Errno 2] No such file or directory ##[error]Process completed with exit code 1.

@adithyabsk
Copy link
Member

@jeffweng8 that's related to this: #233

If you notice, your build passed on Linux. Thanks for the PR, I'll try to do a proper CR in the next week.

@jeffweng8
Copy link
Contributor Author

Hey @adithyabsk have you had a chance to look at the changes yet?

@jeffweng8
Copy link
Contributor Author

jeffweng8 commented Jun 20, 2020

Hi @adithyabsk , Are you still planning on working on or maintaining this project?

@adithyabsk
Copy link
Member

@jeffweng8 Sorry for the long delay in response, I'll look at this in the coming week.

@stale
Copy link

stale bot commented Aug 21, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 21, 2020
@stale
Copy link

stale bot commented Aug 28, 2020

Closing this pull request automatically because it has not had any activity since it has been marked as stale. If you think it is still relevant and should be addressed, feel free to open a new one.

@stale stale bot closed this Aug 28, 2020
@adithyabsk adithyabsk reopened this Nov 26, 2022
@stale stale bot removed the stale label Nov 26, 2022
@adithyabsk adithyabsk mentioned this pull request Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants