-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: add transaction export as JSON #50
base: master
Are you sure you want to change the base?
Conversation
d935d67
to
fb25982
Compare
fb25982
to
478fa8f
Compare
pytr/main.py
Outdated
description=info, | ||
) | ||
transactions.add_argument( | ||
'output', help='Output path of CSV file', metavar='OUTPUT', type=Path) |
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.
For some commands output is a file, sometimes a folder. That may be confusing, no?
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 think I can't quite follow this comment.
I took a look at export_transactions
when writing this, where the file is specified, too.
I prefer that over only specifying a folder, because it is easy to use in scripts.
Thanks for your feedback, I'll address it in the upcoming days! |
478fa8f
to
6dfcbda
Compare
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.
@Katzmann1983 I added all your suggestions, thanks for the detailed review!
Sorry for the force push here, I only realized once I had already pushed.
My only question is in regards to the output parameter, I don't think I quite got your comment there.
pytr/main.py
Outdated
description=info, | ||
) | ||
transactions.add_argument( | ||
'output', help='Output path of CSV file', metavar='OUTPUT', type=Path) |
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 think I can't quite follow this comment.
I took a look at export_transactions
when writing this, where the file is specified, too.
I prefer that over only specifying a folder, because it is easy to use in scripts.
This adds a
transactions
command that is similar to theexport_transactions
command.However,
transactions
uses thetimelineTransactions
, which contain all transactions and exports the JSON as seen on the websocket to a target file.