issue #109 Replace print statements with logger #113
Conversation
|
Hi @aaronchall, Before I can review this more thoroughly we need to ensure it passes tests. It seems that there is an error in the log usage: https://travis-ci.org/pypa/auditwheel/jobs/447196891#L538-L540 Can you please run the tests and work to fix this? I'm not sure at a quick glance why this is failing. |
|
@ehashman my apologies, we don't use github/travis where I work, most of my PR's in the past year have been for docs, and I haven't used this workflow in a long time. Tests are now passing, though! |
| @@ -46,12 +50,12 @@ def execute(args, p): | |||
| try: | |||
| out_wheel = add_platforms(ctx, [wheel_abi.overall_tag]) | |||
| except WheelToolsError as e: | |||
| print('\n%s.' % str(e), file=sys.stderr) | |||
| logger.exception('\n%s.', repr(e)) | |||
|
@ehashman please let me know what you think of these changes! We just need to set the logging levels for pytest (which requires pytest 3.4 or higher, I believe). |
|
This PR is getting a little long in the tooth so I pushed some commits to your branch that accomplish the same as what you're intending here but with less code and hardcoded configs. |
There was a problem hiding this comment.
Okay, I am super into this. The logger means that we don't have to scatter useless print statements throughout the code and we can correctly start embedding debug statements as necessary, and tune test output by manually passing --log-cli-level (e.g. = 10 for DEBUG).
And our test output is now looking a lot nicer! Big win, I'm going to squash and merge. 💯
|
@ehashman Thanks, I would have just done it as INFO/ level 20 myself but I was worried it would be too verbose for the maintainers, but I agree this is better!! I didn't realize you could configure pytest in setup.cfg either! Cheers!!! |
Thanks!
Fixes #109.