-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
add option to query account details including personal data #159
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @marcusfey
pytr/accountdetails.py
Outdated
@@ -0,0 +1,16 @@ | |||
import json | |||
|
|||
class Accountdetails: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class Accountdetails: | |
class AccountDetails: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this class has any right to be here, it's such a thin wrapper. :)
What would be neat is this was a TypedDict
instead that describes the fields returned by TradeRepublicApi.get_account_details()
.
pytr/main.py
Outdated
elif args.command == "accountdetails": | ||
ad = Accountdetails( | ||
login(phone_no=args.phone_no, pin=args.pin, web=not args.applogin), | ||
) | ||
ad.get() | ||
if args.jsonoutput is not None: | ||
ad.data_to_file(args.jsonoutput) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif args.command == "accountdetails": | |
ad = Accountdetails( | |
login(phone_no=args.phone_no, pin=args.pin, web=not args.applogin), | |
) | |
ad.get() | |
if args.jsonoutput is not None: | |
ad.data_to_file(args.jsonoutput) | |
elif args.command == "accountdetails": | |
tr = login(phone_no=args.phone_no, pin=args.pin, web=not args.applogin) | |
account_details = tr.get_account_details() | |
args.jsonoutput.write(json.dumps(account_details)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question here: this would write the json args.jsonoutput
without checking if this parameter is actually set.
Are the any implicit checks I'm not aware of? Or should I change this to
elif args.command == "accountdetails":
tr = login(phone_no=args.phone_no, pin=args.pin, web=not args.applogin)
account_details = tr.get_account_details()
if args.jsonoutput is not None:
args.jsonoutput.write(json.dumps(account_details))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other change above I suggested makes it so args.jsonpath
is always set, to a file-like object in write mode. If no argument is specified, it defaults to -
which argparse.FileType
interprets as "stdout".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right! I wasn't aware initially that those -o
parameter are per command. It's a nice solution.
Thanks for your review! I'm completely fine with what you requested. Actually for me Thus: Should I remove |
Yes, I think that's okay! If you care to invest the time, it would be nice to have a |
I added the TypedDict with 1ce3145. But I really not sure if that's what you had in mind 🫣 |
The API so far does not read the account data from Traderepublic. A lot of this data is not that interesting and pretty static (name, address, etc). But there is also the main account including IBAN.
I need this for my own asset management application where I aggregate all the current/saving accounts from all my banks. To achieve this I added modules for each bank, they send the account information to the main app. Thus only the balance is not enough, at least IBAN is needed to sort things out for the main app.
I'm aware that returning the dict and writing out the JSON is not that much of a processing. Please consider adding this "anyway", change the code or tell me what I should add.
Addmittedly I'm pretty new to Python.
This is roughly what you will get: