-
Notifications
You must be signed in to change notification settings - Fork 4
Support stderr #33
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
Support stderr #33
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.
Looks good, just a couple of questions inline.
pytroll_runner/__init__.py
Outdated
out, err = process.communicate() | ||
logger.debug(f"After having run the script: [stdout]{out} [stderr]{err}") | ||
if err: | ||
return out + err |
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.
Would it hurt to remove the "if" and always return out + err
?
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.
Done!
pytroll_runner/tests/test_runner.py
Outdated
@pytest.fixture(params=[False, True]) | ||
def dumped_to_std_err(request): | ||
return request.param |
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.
Really nice with request.param
, works well for this use case!
One thing though: I think it might be even better to use eg sys.stdout
and sys.stderr
as values here. In which case, the fixture should probably change name, eg "output_stream"?
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.
Done!
also, pre-commit isn’t happy. |
f7172b6
to
8c33b86
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.
LGTM
Co-authored-by: Martin Raspaud <[email protected]>
No description provided.