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

Refactored tdclient with session handler and exceptions #50

Merged
merged 7 commits into from Mar 20, 2020

Conversation

dahifi
Copy link
Collaborator

@dahifi dahifi commented Mar 20, 2020

This is a pretty big design change, I put a lot of work into it.

I use a design pattern described by Ben Homnick (http://bhomnick.net/design-pattern-python-api-client/) to separate the transport request from the application logic. The TDClient now uses a TDASession class, which inherits from requests. It doesn't do much right now, but my plan is to move the authentication functionality to it at some point. This will handle tokens and auto refresh, and should be able to handle the request timeout that someone was requesting. 

The client _request function passes all the various methods to the session, and will throw exceptions if the response is anything other than a 200, or 201 as happens when placing orders. This check will probably get moved into session so that it can try refreshing the token when 401s are thrown.

I added tests for as much as I could; I'm starting to get a better handle on mock, so I think this is moving in the right direction. 

dahifi and others added 6 commits February 1, 2020 09:24
Merging my fork to head repo
This is a basic stub function, I don't have tests cause I didn't TDD for these.
I have additional supporting libraries that add the rest of the functionality to complete an order,
but they depend on the QuickType library that I've developed. It's too much for one commit to add all that to this
library. I need to figure out a way to integrate them into this library, and I will attempt
to add them as soon as I have time.
TDAmeritrade has some sample JSON data in their guides section that shows valid schema. https://developer.tdameritrade.com/content/place-order-samples

Added the JSON string to its own file and imported as a fixture to be used by test_orders and test_saved_orders
I use a design pattern described by Ben Homnick (http://bhomnick.net/design-pattern-python-api-client/) to separate the transport request from the application logic. The TDClient now uses a TDASession class, which inherits from requests. It doesn't do much right now, but my plan is to move the authentication functionality to it at some point.

The client _request function passes all the various methods to the session, and will throw exceptions if the response is anything other than a 200, or 201 as happens when placing orders. This check will probably get moved into session so that it can try refreshing the token when 401s are thrown.

I added tests for as much as I could; I'm starting to get a better handle on mock, so I think this is moving in the right direction.
@dahifi
Copy link
Collaborator Author

dahifi commented Mar 20, 2020

Not sure why the fails, I assume it's something with the new CI, cause i doesn't look like the tests were actually executed. I originally ran 3.6.9 on my environment but just pulled 3.7 and everything passed as well.

@timkpaine
Copy link
Owner

did you run lint?

flake8 tdameritrade 
tdameritrade/client.py:101:29: E128 continuation line under-indented for visual indent
tdameritrade/client.py:159:9: F841 local variable 'resp' is assigned to but never used
tdameritrade/client.py:168:38: W292 no newline at end of file
tdameritrade/exceptions.py:53:9: W292 no newline at end of file

Restored the 'return resp' pattern for consistency. Newline and continuation indentation issues corrected.
@timkpaine timkpaine merged commit 6ca8a92 into timkpaine:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants