Skip to content

Conversation

@bluetech
Copy link
Member

@bluetech bluetech commented Nov 8, 2025

Fix #13896

@bluetech bluetech added the backport 9.0.x apply to PRs at any point; backports the changes to the 9.0.x branch label Nov 8, 2025
@psf-chronographer psf-chronographer bot added bot:chronographer:provided (automation) changelog entry is part of PR labels Nov 8, 2025
@bluetech bluetech force-pushed the iterm-terminalprogress branch from f2316b3 to c37e687 Compare November 8, 2025 22:34
Comment on lines 302 to +311
if reporter.isatty():
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")
# Some terminals interpret OSC 9;4 as desktop notification,
# skip on those we know (#13896).
should_skip_terminal_progress = (
# iTerm2 (reported on version 3.6.5).
"ITERM_SESSION_ID" in os.environ
)
if not should_skip_terminal_progress:
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")
Copy link
Member

Choose a reason for hiding this comment

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

This feels overly nested. How about

Suggested change
if reporter.isatty():
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")
# Some terminals interpret OSC 9;4 as desktop notification,
# skip on those we know (#13896).
should_skip_terminal_progress = (
# iTerm2 (reported on version 3.6.5).
"ITERM_SESSION_ID" in os.environ
)
if not should_skip_terminal_progress:
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")
# Some terminals interpret OSC 9;4 as desktop notification,
# skip on those we know (#13896).
should_skip_terminal_progress = (
# iTerm2 (reported on version 3.6.5).
"ITERM_SESSION_ID" in os.environ
) or not reporter.isatty()
if should_skip_terminal_progress:
return
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to avoid early return in hooks because it's possible we add more unrelated stuff to it in the future.

Copy link
Member

Choose a reason for hiding this comment

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

So

Suggested change
if reporter.isatty():
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")
# Some terminals interpret OSC 9;4 as desktop notification,
# skip on those we know (#13896).
should_skip_terminal_progress = (
# iTerm2 (reported on version 3.6.5).
"ITERM_SESSION_ID" in os.environ
)
if not should_skip_terminal_progress:
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")
# Some terminals interpret OSC 9;4 as desktop notification,
# skip on those we know (#13896).
enable_terminal_progress = (
# iTerm2 (reported on version 3.6.5).
"ITERM_SESSION_ID" not in os.environ
) and reporter.isatty()
if enable_terminal_progress:
plugin = TerminalProgressPlugin(reporter)
config.pluginmanager.register(plugin, "terminalprogress")

then?

Copy link
Member Author

Choose a reason for hiding this comment

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

It messes with the comment a bit. The isatty is the more important check, but I want to have a dedicated comment for the entire "unsupported terminals" check (in case we decide to add more cases).

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

Labels

backport 9.0.x apply to PRs at any point; backports the changes to the 9.0.x branch bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

terminalprogress causes thousands of iTerm2 alerts

2 participants