-
Notifications
You must be signed in to change notification settings - Fork 134
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
[issue-441] replace print() with logging #494
[issue-441] replace print() with logging #494
Conversation
ca56361
to
506aec2
Compare
Signed-off-by: Armin Tänzer <[email protected]>
506aec2
to
70b3079
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.
Your changes look good to me. I found two additional places in the codebase where information should bel logged. As I can't comment on unchanged lines, I reference them here:
- the output to the console is a logging.info
- the system exits here should also be logged
Within the parser I also implemented a Logger
- class, I didn't look in the documentation for the logging
package but do you know if we could merge this somehow? So does the logging
package provide any options for an individual logger? Although I am not sure if we would mix different levels of logging when merging this.
Signed-off-by: Armin Tänzer <[email protected]>
68ce27e
to
2850fd3
Compare
I fixed the system exits' logs, but for the cli tool parser the output is the actual wanted result of the tool and should therefore go to stdout, I think. |
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 addressing my remark and I agree that the output is wanted as is.
Do you have any thoughts on my last question concerning an indiviual logger?
Concerning the parsing logger, which basically just collects all parsing errors, I think that it is still valid as it is to give helpful information for the |
fixes #441