jessecooper / pyetrade

Python E-Trade API Wrapper
GNU General Public License v3.0
205 stars 96 forks source link

Kwargs expansion, linting, and formatting #89

Closed Robert-Zacchigna closed 5 months ago

Robert-Zacchigna commented 8 months ago

Hello again, so it seems I was finally motivated enough to expand and lint kwargs for the etrade api.

THIS WILL BE A BREAKING CHANGE

I expanded all methods using kwargs (except in order.py, that will have to be done another day...) with the api variables and their respective defaults.

Since before you had to use the specific name of the api variable to be expanded by kwargs, the variable names had to be slightly changed to fit python formatting standards.

EXAMPLES:

So anyone using kwargs before this point will need to update their variable names to match the ones now added to the methods.

I also applied some spelling, grammar and other general formatting fixes.

@jessecooper @1rocketdude Please review and let me know if anything needs to be changed/fixed. This should hopefully make understanding and using the various methods much easier for people.

codecov-commenter commented 8 months ago

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.22%. Comparing base (17a8957) to head (21050b4).

Files Patch % Lines
pyetrade/accounts.py 90.90% 1 Missing :warning:
pyetrade/order.py 75.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #89 +/- ## ========================================== + Coverage 87.28% 90.22% +2.93% ========================================== Files 6 6 Lines 401 409 +8 ========================================== + Hits 350 369 +19 + Misses 51 40 -11 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jessecooper commented 7 months ago

@Robert-Zacchigna Thanks for the PR and apologies for the lack of response. Give me some time to review and test these changes.

Robert-Zacchigna commented 6 months ago

@jessecooper just checking in, any issues and/or changes you think i need to make?

jessecooper commented 6 months ago

I apologize I have not had a moment to give this a full review. It is next on my backlog. I have not forgotten I promise.

csinko commented 6 months ago

THIS WILL BE A BREAKING CHANGE

I would bump version to at least 1.5 then

jessecooper commented 5 months ago

@Robert-Zacchigna This is a great PR. Thank you for taking your time to make these much needed changes and also cleaning things up and added type annotations. It is very appreciated. Please see comment: https://github.com/jessecooper/pyetrade/pull/89/files#r1526233328. Either you can just change the file back and I can handle the bump after the merge or you can do the bump either way will work for me.

Robert-Zacchigna commented 5 months ago

@Robert-Zacchigna This is a great PR. Thank you for taking your time to make these much needed changes and also cleaning things up and added type annotations. It is very appreciated. Please see comment: https://github.com/jessecooper/pyetrade/pull/89/files#r1526233328. Either you can just change the file back and I can handle the bump after the merge or you can do the bump either way will work for me.

No problem at all, I'm glad you found the PR to be of good quality.

I have reverted my version bump, you are free to handle that as you see fit.

I'm sure I'll mosey on back to this again to finish order.py when i have more free time again (if you don't beat me to do it).

jessecooper commented 5 months ago

This is released in pyetrade 2.0.1