Skip to content
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

Subprocess stream_output=False fixes #1056

Merged
merged 1 commit into from Jan 23, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Jan 22, 2023

Changes

  • When steam_output=False, disable dynamic console elements such as the Wait Bar.
  • Remove condition of console control or output redirection for output streaming a command. These are irrelevant since output streaming will happen regardless unless stream_output=False...in which case console control will be released anyway now.

Related Issues

Notes

Looking at the tests and so forth (especially those in test_Subprocess__run__controlled_console.py), it seems clear that stream_output=False was not expected to actually disable streaming if the console was currently controlled. This was the wrong implementation.....but one that was probably based on the assumption that upstream code would not invoke commands that could not be streamed at the same time the console was controlled (e.g. running an unstreamable command in the Wait Bar). Earlier on, this was probably a reasonable assumption but we've made the framework much more flexible and accommodating since then.

Additionally, when #943 updated the default for stream_output from False to True, it probably should have also reconsidered downstream logic assuming stream_output would usually be False.

These changes should help streamline all this now.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

- Disable dynamic console elements such as the Wait Bar when
  `steam_output=False`.
- Remove condition of console control or output redirection for output
  streaming a command. These are irrelevant since output streaming will
  happen regardless unless `stream_output=False`...in which case console
  control will be released anyway now.
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense; it took me a bit to get comfortable with the change of logic around making _run_and_stream_output entirely controlled by stream_output, but I think the new setup works - it will just raise errors in different ways (but not in any usage that we're actually exercising).

@freakboy3742 freakboy3742 merged commit 9233555 into beeware:main Jan 23, 2023
@rmartin16 rmartin16 deleted the respect-steam_output=False branch January 23, 2023 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Use of stream_output=False is not Respected within a Controlled Console
2 participants