Skip to content

Conversation

epage
Copy link
Contributor

@epage epage commented Oct 6, 2025

To write colored output on a locked stream, you had to:

  1. Call process.stdout() to get a Box<dyn Writer>
  2. Call Writer::terminal(process) to get a ColorableTerminal (enum)
  3. Call ColorableTerminal::lock to get a ColorableTerminalLocked (enum)

This cuts out the first step, making sure everything goes through ColorableTerminal.

epage added 3 commits October 6, 2025 12:13
In a lot of cases, `Process` will create a boxed `Writer` just then to
create a `ColorableTerminal` from it.
This bypasses some of those steps, simplifying the code.
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Great stuff!


fn terminal(&self, process: &Process) -> ColorableTerminal {
ColorableTerminal::new(StreamSelector::TestWriter(self.clone()), process)
ColorableTerminal::test(self, process)
Copy link
Member

@rami3l rami3l Oct 7, 2025

Choose a reason for hiding this comment

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

Question: is it true that this line has been changed here first and a second time in the following commit? Would that mean at least one of the commits are not compiling, thus violating the rules of atomic commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right; I forgot to test my history edits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@rami3l rami3l Oct 8, 2025

Choose a reason for hiding this comment

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

@epage Sorry, but I don't see the fix being applied here? I think .clone() is needed from the start if I'm not mistaken. Maybe you are not checking with the test feature on?

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.

3 participants