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

Disable progress bar and wait bar if stdout is not attached to a tty #1267

Merged
merged 1 commit into from
May 9, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented May 8, 2023

Changes

  • Disables the Progress Bar and Wait Bar if stdout if not attached to a tty

Notes

  • It was discovered in Use FORCE_COLOR to enable colored output in CI .github#36 that when FORCE_COLOR is set, Rich not only enables ANSI color and styling, but also turns on "live" elements like the Progress Bar
  • Allowing the Progress Bar to run in CI can create really ugly logs
  • I initially considered disabling these dynamic elements when the --no-input switch is used....but I decided against it because:
    • It's a weak proxy for "non-interactive session"
    • It can disable the dynamic elements when they are still desired and reasonable to show
    • When users encounter the problem of polluted CI logs, they would be required to add an un-intuitive switch to all invocations of Briefcase
  • Rich's recommendation for this situation is to instantiate Console with force_terminal=True and force_interactive=False
    • However, this recommendation is qualified with "you might want to do this in some CI environments"....and this only seems to make sense if the application using Rich is only really used in CI by the application developer
    • In Briefcase's case, we would need to establish some process for users to implement in their CI in order to activate these overrides for Console
    • Alternatively, we could always connect sys.stdout.isatty() to these overrides....although, I think I'd rather just let Rich do what it does by default and just target our known issues
      • After all, Rich has all the battle-tested logic for when terminal and interactive things should happen
  • Thus, just disabling the specific uses of Rich's dynamic elements in Briefcase when a tty isn't available seems like the best way to use default functionality while avoiding known issues

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

@rmartin16 rmartin16 force-pushed the force-colors branch 3 times, most recently from 9682101 to 4a1a0ae Compare May 8, 2023 18:56
@rmartin16 rmartin16 marked this pull request as ready for review May 8, 2023 18:57
@rmartin16 rmartin16 force-pushed the force-colors branch 2 times, most recently from 4435c45 to b9d9ba6 Compare May 8, 2023 19:11
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.

Implementation looks good; one suggestion about the change note that I wanted to confirm before merging.

changes/1267.bugfix.rst Show resolved Hide resolved
@freakboy3742 freakboy3742 merged commit f339885 into beeware:main May 9, 2023
@rmartin16 rmartin16 deleted the force-colors branch May 9, 2023 02:11
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.

2 participants