-
Notifications
You must be signed in to change notification settings - Fork 604
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
base: master
Are you sure you want to change the base?
Add option base model #224
Conversation
- todo: add **kwargs for more criteria on matching
-make its own endpoint using instruments endpoint
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. |
Hey @adithyabsk I am getting |
@jeffweng8 I think you might not have the packages installed locally, perhaps? (or at least not in a place mypy can find them) |
Ah that makes sense, I just installed yarl and marshmallow in the virtual environment and that fixed the issue. Thanks! |
@jeffweng8 you likely want to install all of our dev dependencies either using |
Hmm I am getting this error when I try to run the poetry install command: Traceback (most recent call last): Any idea where that is coming from? I ran Also, when I try to run pytest, I get the following: Is this related to the above issue? |
Also should I close this PR and make a new PR where I squash my commits? |
Nope, no worries. I can squash when I merge @jeffweng8 |
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) |
Additionally if you use poetry it will automatically manage your virtual environment so you can just do |
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. |
@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. |
…pyrh into add_options_endpoints
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 |
@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. |
Also, what specifically is broken? |
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? |
SessionManager allows you to authenticate with the Robinhood API. Take a look at how I structured the Portfolio modules. I made a Eventually, we should be able to mark specific methods as not requiring log-in which should work without the authentication flow. |
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:
|
@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. |
Hey @adithyabsk have you had a chance to look at the changes yet? |
Hi @adithyabsk , Are you still planning on working on or maintaining this project? |
@jeffweng8 Sorry for the long delay in response, I'll look at this in the coming week. |
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. |
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. |
Checklist
"{ISSUE_NUM}.{feature|bugfix|doc|removal|misc}""
Related Issue
#201
Description