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

Use correct stdout stream in publish_module. #256

Merged
merged 5 commits into from Mar 28, 2018

Conversation

Projects
None yet
3 participants
@icemac
Member

icemac commented Mar 21, 2018

If publish_module is called with a _response parameter which is not None its stdout is written to but is not read from.

Detected in zopefoundation/Products.ExternalEditor#10.

Use correct stdout stream.
If `publish_module` is called with a `_response` parameter which is not `None` its stdout is written to but is not read from.

Detected in zopefoundation/Products.ExternalEditor#10.
@hannosch

There are two test failures as a result of this change. Some test mockup object doesn’t have a stdout attribute. Otherwise this looks ok. Maybe adjust the comment above the changed line, to better reflect the new logic.

icemac added some commits Mar 21, 2018

@icemac

This comment has been minimized.

Member

icemac commented Mar 21, 2018

@hannosch I fixed the tests (I did no expect them to exist – shame on me!) but they helped me to write a test which breaks without the change. Are you fine with my changes?

@icemac

This comment has been minimized.

Member

icemac commented Mar 27, 2018

@hannosch Ping.

@tseaver

This comment has been minimized.

Member

tseaver commented Mar 27, 2018

@icemac I think you can go ahead with the merge: I just verified that a8850ca and eab160c address the review comments from @hannosch .

@tseaver verified that the review comments were addressed.

@icemac icemac merged commit 20d92cc into master Mar 28, 2018

1 check was pending

continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@icemac icemac deleted the icemac-patch-1 branch Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment