-
Notifications
You must be signed in to change notification settings - Fork 9
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 CLI flag to specify output file #44
Comments
Hi @aa-ko, first of all that sounds like a good idea. Can you share a little bit more about the error that you are getting during the 2FA? Maybe we need to improve the error handling in this case as well. Also, what would be a valid output in case an error occurs? Thanks for your interest in the project and for submitting the issue. |
Hi @jsattler, thanks for the quick reply! Let me rephrase this a little bit. The second case will always be a problem, even if I am fast enough to re-authenticate in time, because the info message will be written anyways if the token is no longer valid. As for improving the error handling, I don't think there is any need to change the current code. All messages I had problems with are correct and useful, they are just at the wrong place, if I happen to redirect stdout to a file. In my opinion, the correct behavior would be to simply not write to file if an error occurs and to still print all messages to stdout/stderr. |
Hi! I ran into an issue when trying to use the CLI in a script.
If the tool generates an error message while waiting on the 2FA authentication to complete, this error will be written to stdout along with the (CSV/JSON) output from the comdirect API.
In my case, the tool is called like this:
# comdirect depot position <id> -f csv > depot.csv
This sometimes results in an invalid CSV file, where the first line contains the error message.
This is why I propose the addition of a new
-o / --output
flag, that will always create a valid output file while still printing any log message to stdout/stderr.My implementation of this is working fine and I'm happy open a PR with my changes after some more testing and if this is something you'd like to support :)
The text was updated successfully, but these errors were encountered: