Initial fixes run through: prev/place order, type hinting and general formatting

Robert-Zacchigna commented 1 year ago

@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:

{
    "Error": {
        "code": "1021",
        "message": "We cannot accept this order because there are insufficient funds in your account."
    }
}

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.

xozxro commented 1 year ago

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.

jessecooper commented 1 year ago

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.

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.

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...

jessecooper commented 1 year ago

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.

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.

jessecooper commented 1 year ago

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.

jessecooper commented 1 year ago

@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. https://docs.python.org/3/library/typing.html#typing.Union

I am willing to merge this even without the custom exception but I would like to put a #TODO comment on that if you do not want to create the custom exception. Otherwise it is just trying to removed the session import if we can avoid the import for type.

Robert-Zacchigna commented 1 year ago

@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:

I updated the imports to reduce the overhead a bit, since for several of them its only using one function from the package.
I remediated the Union issue (i think) and explicitly imported the Union type, didn't realize the shorthand was only recently added in 3.10
Lastly, i updated the get_request_result function to handle the Etrade API errors, I'm simply checking if the 'Error' key is present and then outputting the error code and message (let me know if you want something more refined):
- Example Output (attempting to trade after hours):
- Exception: Etrade API Error - Code: 7029, Msg: The order you entered is not valid at this time. Please contact Customer Service at 1-800-387-2331 for assistance.

Let me know what you think.

Robert-Zacchigna commented 1 year ago

@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-commenter commented 1 year ago

Codecov Report

Patch coverage: 79.51% and project coverage change: +0.94 :tada:

Comparison is base (13f07c4) 86.34% compared to head (0a8cc83) 87.28%.

:mega: 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

```diff @@ 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 ``` | [Impacted Files](https://codecov.io/gh/jessecooper/pyetrade/pull/78?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jesse+Cooper) | Coverage Δ | | |---|---|---| | [pyetrade/order.py](https://codecov.io/gh/jessecooper/pyetrade/pull/78?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jesse+Cooper#diff-cHlldHJhZGUvb3JkZXIucHk=) | `77.84% <79.51%> (+1.70%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jesse+Cooper). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jesse+Cooper)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jessecooper commented 1 year ago

@Robert-Zacchigna Thank you for your PR and patients! It is very appreciated.

Robert-Zacchigna commented 1 year ago

No problem at all, this has been learning experience for me as well.

jessecooper commented 1 year ago

This has been released in v1.4.1 on PyPI

jessecooper / pyetrade

Initial fixes run through: prev/place order, type hinting and general formatting #78

Codecov Report