-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Initial fixes run through: prev/place order, type hinting and general formatting #78
Conversation
This is good. My company is running into this issue and finding this pull request is the only thing I found to figure it out. Suggest that this is dealt with ASAP. Thanks for the great work. |
My apologies for the lag on this. Had to wrap up things in the day job before taking vacation. I am now getting back into the swing of things.
For the kwargs. I am fine with not untangling all of that at this time. Some of the APIs have a lot of args and trying to convert and cover them all will take time. Starting this review on the GitHub app from my phone so I'll have to make this a multi post comment... |
Here instead of using the raise from status we should look at the status code and if it is greater than 200 raise a custom exception with the E-Trade error text and status code. I will start the full code review tomorrow night. Thanks you again for the PR and the patience with the comments and review. |
Ok it's a bit harder to review a change on the phone in the GitHub app. I'll be back at an actual screen come Monday but it looks good so far just want to give it a close look. |
@Robert-Zacchigna It looks good. The tests are only failing right now because the Union type was introduced in Python 3.10 but I am fine with deprecating support for older versions of python. I am willing to merge this even without the custom exception but I would like to put a |
@jessecooper Hey, ya no problem at all, i was actually just about to post another message just in case you forgot (all good). Please review my latest commit:
Let me know what you think. |
… 'test_place_equity_order'
@jessecooper See latest commit, unit-tests should all pass now, took me a little bit to setup my local env to test them all but it should be all good now. Please double check that the ones i changed are still setup correctly. |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #78 +/- ##
==========================================
+ Coverage 86.34% 87.28% +0.94%
==========================================
Files 6 6
Lines 410 401 -9
==========================================
- Hits 354 350 -4
+ Misses 56 51 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@Robert-Zacchigna Thank you for your PR and patients! It is very appreciated. |
No problem at all, this has been learning experience for me as well. |
This has been released in v1.4.1 on PyPI |
@jessecooper This is my initial run through to fix the prev/place order. I added type hinting basically everywhere and fixed some formatting.
Another thing i noticed while i was testing things, if you try to preview/place an order of something that is more than the current value of available cash in your account, the API returns an error. As a result, line 52 of
order.py
raises the request error from the etrade API response BUT in doing so doesn't allow the user to see the etrade API error output, thus outputting the HTTP 400 bad response error instead.This is what the error code would like from the etrade API:
After looking through peoples issues, i almost guarantee that this is the main issue people were facing. The API for whatever reason won't even let you preview the order if you don't have enough funds in your account (happens with my work around functions as well). I've only commented it out for now as I'm not sure how you want to handle the error codes.
Lastly, untangling kwargs appears to be a bigger task than i thought, it would require a rewrite of quite a few things to work correctly with how things currently work. I haven't made those changes as i wanted to get your feedback on all of my work so far and see how you wanted to proceed.
I tried to modify the tests as well but admittedly i was never really good about unittesting, so I might need some help in that department.