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

Add CLI flag to specify output file #44

Open
aa-ko opened this issue Jan 12, 2024 · 2 comments
Open

Add CLI flag to specify output file #44

aa-ko opened this issue Jan 12, 2024 · 2 comments

Comments

@aa-ko
Copy link

aa-ko commented Jan 12, 2024

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 :)

@jsattler
Copy link
Owner

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.

@aa-ko
Copy link
Author

aa-ko commented Jan 12, 2024

Hi @jsattler, thanks for the quick reply!

Let me rephrase this a little bit.
If the timeout is reached before I am able to authenticate the request, a context deadline exceeded message will be written to the file: 2024/01/12 19:56:26 context deadline exceeded
If the 2FA token has expired and I have to authenticate again, I will have this message at the top of my file: Your session expired. Please open the comdirect photoTAN app to validate a new session.

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.
For usage in a script, this can be further improved by returning with specific error codes.

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

No branches or pull requests

2 participants