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?

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