Skip to content

Cows. Cows everywhere. - #135

Open
shanecelis wants to merge 11 commits into
colored-rs:masterfrom
shanecelis:cow
Open

Cows. Cows everywhere.#135
shanecelis wants to merge 11 commits into
colored-rs:masterfrom
shanecelis:cow

Conversation

@shanecelis

@shanecelis shanecelis commented Jun 9, 2023

Copy link
Copy Markdown

Great project. I saw an opportunity to alloc less. This PR changes input from a String to a Cow<'a, str> and the implementation for &'a str to Into<Cow<'a, str>> which will cover &str as before and String and Cows and makes the necessary changes to support that. No usage change for the end user, just broader support and less allocs.

As an aside, I added print statements to the dynamic_colors example.

Comment thread src/control.rs Outdated
Comment on lines +106 to +112
#[allow(unused_mut)]
#[allow(unused_assignments)]
let mut tty = false;
#[cfg(feature = "tty")]
{
tty = atty::is(atty::Stream::Stdout);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
#[allow(unused_mut)]
#[allow(unused_assignments)]
let mut tty = false;
#[cfg(feature = "tty")]
{
tty = atty::is(atty::Stream::Stdout);
}
let tty = if cfg!(feature = "tty") {
atty::is(atty::Stream::Stdout)
} else {
false
};

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good idea. I like this better than mine but it doesn't compile. This compiles:

        let tty = if cfg!(feature = "tty") {
            #[cfg(feature = "tty")]
            {
            atty::is(atty::Stream::Stdout)
            }
            #[cfg(not(feature = "tty"))]
            false
        } else {
            false
        };

But we can clean it up further to this:

        let tty = {
            #[cfg(feature = "tty")]
            {
                atty::is(atty::Stream::Stdout)
            }
            #[cfg(not(feature = "tty"))]
            false
        };

And sorry there are some commits that I didn't mean to put into this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could also use

use std::io::{self, IsTerminal};
let tty = io::stdout().is_terminal();

that was very recently introduced in the Rust 1.70.0 std library.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Could use that and drop the atty dependency.