-
Notifications
You must be signed in to change notification settings - Fork 83
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
[sortinghat] Add logging support #153
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.
Check my comment. The patch looks good to me.
sortinghat/command.py
Outdated
sys.stdout.write(s) | ||
|
||
def error(self, msg): | ||
s = "Error: %s\n" % msg | ||
sys.stderr.write(s) | ||
logger.error(s) |
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 wouldn't change this. We have to make a difference between those messages related to the internal work of the tool (log messages) and those messages generated as an output for the user (which is this case).
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.
ok @sduenas, I'm going to revert this change and update the PR
I have updated the pr according to your comments @sduenas , please have a look |
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.
You forgot to remove the logging call when rendering the output of a command.
sortinghat/command.py
Outdated
@@ -57,7 +60,8 @@ def display(self, template, **kwargs): | |||
|
|||
t = env.get_template(template) | |||
s = t.render(**kwargs) | |||
|
|||
logger.info(s) |
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.
This should be also removed as you did with the errors.
I have updated the PR, including changes due to the inclusion of #155 to the codebase. |
This code aligns test code with the support for logging. The modification was required since after including this support, tests were failing due to the way of checking the outcome of commands (stdin, stderr) which was clashing with the logger output.
This code includes the cryptography package in the travis.yml, in order to make the tests pass.
This PR targets issue #149 . Thus it includes support to for logging.