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

Kwargs expansion, linting, and formatting #89

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

Robert-Zacchigna
Copy link
Contributor

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:

  • sortBy becomes sort_by
  • startDate becomes start_date
  • sortOrder becomes sort_order
  • etc...

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
Copy link

codecov-commenter commented Jan 6, 2024

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 ⚠️
pyetrade/order.py 75.00% 1 Missing ⚠️

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jessecooper
Copy link
Owner

@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
Copy link
Contributor Author

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

@jessecooper
Copy link
Owner

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
Copy link

csinko commented Mar 2, 2024

THIS WILL BE A BREAKING CHANGE

I would bump version to at least 1.5 then

@jessecooper jessecooper self-assigned this Mar 14, 2024
setup.py Outdated Show resolved Hide resolved
@jessecooper
Copy link
Owner

@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
Copy link
Contributor Author

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

Copy link
Owner

@jessecooper jessecooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM;

@jessecooper jessecooper merged commit aec87da into jessecooper:master Mar 22, 2024
10 of 14 checks passed
@jessecooper
Copy link
Owner

This is released in pyetrade 2.0.1

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.

4 participants