Make current_terminfo a OncePerProcess #58854
Conversation
There seems to be no reason to always load this unconditionally - especially since it's in the critical startup path. If we never print colored output or our IO is not a TTY, we don't need to load this at all. While we're at it, remove the `term_type` argument to `ttyhascolor`, which didn't work as advertised anyway, since it still looked at the current_terminfo. If clients want to do a full TermInfo check, they can do that explicitly.
|
JuliaLang/StyledStrings.jl#121 for the StyledStrings side of this. I'll leave it like this for review. If accepted, I'll update this PR to bump to the StyledStrings version to run integrated CI, then merge this, then merge StyledStrings, but that's a bit of an annoying dance, so leaving this like this for review for the time being. |
Julia base is changing current_terminfo from a simple global variable to a OncePerProcess type. This commit updates all usage sites to call current_terminfo() as a function instead of accessing it directly. This is the counterpart to <JuliaLang/julia#58854>. For tests, we need to directly manipulate the OncePerProcess' internal value field, which relies on undocumented internals but is necessary for testing different terminal configurations without restarting the process.
There was a problem hiding this comment.
The changes here all look pretty sensible 👍. Loading the terminfo should be pretty quick (~30us on my machine), so I'd be half-tempted to label this a "premature optimisation" but there's no harm in skipping this small amount of work if it's never needed.
If you want to bundle in the paired bump with this PR, you can set StyledStrings's commit hash to 1aafc2f3abb6a977ee36a87206f9ce6446a8ae86. I can fast-forward merge the open PR, and then push that commit to main so that everything's happy and in sync.
|
The big advantage of this is the localization of the dependency. Currently for trimming inits have to be executed regardless because they have non local effects, this makes them local |
|
Oh, this makes it trimable? That's cool. |
There seems to be no reason to always load this unconditionally - especially since it's in the critical startup path. If we never print colored output or our IO is not a TTY, we don't need to load this at all. While we're at it, remove the
term_typeargument tottyhascolor, which didn't work as advertised anyway, since it still looked at the current_terminfo. If clients want to do a full TermInfo check, they can do that explicitly.(Written by Claude Code)