Skip to content

Public color API#46

Merged
KodrAus merged 11 commits into
masterfrom
feat/public-color-api
Jan 7, 2018
Merged

Public color API#46
KodrAus merged 11 commits into
masterfrom
feat/public-color-api

Conversation

@KodrAus
Copy link
Copy Markdown
Collaborator

@KodrAus KodrAus commented Dec 22, 2017

Closes #45 and #42

Some thoughts

  • Do we want to treat new environment variables as breaking changes? Users overriding environment variables would probably want to know when new ones are added so they can be overridden too
  • Should Env be an &mut self builder like the others?
  • Should Style implement Write so you can call writeln!(read_style, "{}", record.args()) to write an entire line using styles?

Refactorings

I've moved all the terminal writing related code, like Target and the new WriteStyle into the fmt module to try and encapsulate it a bit better. Now we don't depend on termcolor directly in the Logger or Builder.

Disabling colors

Adds a new builder property for whether or not to include colors. By default this is RUST_LOG_STYLE, but it can be overridden.

Setting this environment variable to never will disable colors and other styles:

$ export RUST_LOG_STYLE=never
$ ./my-app

Valid values are:

  • auto (or missing/invalid) will decide whether or not the terminal supports colors
  • always will always use colors
  • never will never use colors

In order to support multiple environment variables, I've refactored our from_env functions to accept a generic T: Into<Env>, where Env is a container for the environment variables we care about. These methods can now be called in a few ways:

// reads filters from `MY_LOG` and styles from `RUST_LOG_STYLE`
env_logger::init_from_env("MY_LOG");

// reads filters from `MY_LOG` and styles from `MY_LOG_STYLE`
env_logger::init_from_env(Env::default().filter("MY_LOG").write_style("MY_LOG_STYLE"));

This lets us add new environment variables in the future without potentially breaking people. But it does mean if you're overriding all environment variables that new ones could slip in without you noticing.

Using alternative environment variables

Since we use two environment variables to configure the logger we need an ergonomic way to pass different combinations of those variables to from_env methods. This PR adds an Env type with builder methods for naming environment variables:

env_logger::init_from_env(Env::new().filter("MY_LOG"));

With a few From conversions, the above is also equivalent to:

env_logger::init_from_env("MY_LOG");

Whether or not we want to keep these conversions is up for discussion.

Writing colors

The color API has been refactored so it's a bit nicer to use in your own formats:

let mut style = buf.style();

style.set_color(Color::Red).set_bold(true).set_bg(Color::White);

writeln!(buf, "{}", style.value(42))

This saves you from having to split the writes into multiple calls and juggle Result types.

Writing timestamps

Oh the Formatter.timestamp method is now also public so you can grab timestamps.

Copy link
Copy Markdown
Collaborator Author

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Needs some more work to smooth out the rough edges.

Comment thread src/lib.rs Outdated
let mut builder = Builder::new();
let Env { filter, write_style } = env.into();

if let Ok(s) = env::var(&*filter) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Repeating this check is a bit nasty. I think we should add methods to Env to get an Option for each variable

Comment thread src/lib.rs Outdated

if tl_buf.is_none() {
*tl_buf = Some(Formatter::new(self.writer.buffer()));
*tl_buf = Some(Formatter::new(self.writer.buffer(), self.write_style));
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is problematic because multiple loggers working on the same thread with different configurations will get unexpected results. The problem is the same with the Buffer, since it's tied to the Logger.

I'm thinking we just prevent you building a Logger directly and require it's configured globally, but am open to other suggestions.

@KodrAus
Copy link
Copy Markdown
Collaborator Author

KodrAus commented Dec 24, 2017

Looks like we could handle the colors better by using termcolor's ColorChoice enum better. That way we won't need to thread the extra state around.

@KodrAus KodrAus mentioned this pull request Dec 28, 2017
@KodrAus
Copy link
Copy Markdown
Collaborator Author

KodrAus commented Dec 28, 2017

We should align the values of RUST_LOG_STYLE with Cargo's --color inputs.

@KodrAus
Copy link
Copy Markdown
Collaborator Author

KodrAus commented Jan 2, 2018

I've done some refactoring so all the code around writing to the terminal is contained in the fmt module. This means the root Logger and Builder are mostly delegating to the filter and fmt modules to work.

I think this will make the library easier to navigate and use in the future since things are getting more complex in env_logger.

Comment thread examples/custom_format.rs Outdated
Also try setting the `MY_LOG_STYLE` environment variable to `0` to disable colors:

```no_run,shell
$ export MY_LOG_STYLE=0
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This value is wrong. It should be auto, always or never.

Comment thread examples/custom_logger.rs Outdated
Also try setting the `RUST_LOG_STYLE` environment variable to `0` to disable colors:

```no_run,shell
$ export RUST_LOG_STYLE=0
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same as other style examples.

Comment thread examples/default.rs Outdated
Also try setting the `RUST_LOG_STYLE` environment variable to `0` to disable colors:

```no_run,shell
$ export RUST_LOG_STYLE=0
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same as other style examples.

Comment thread src/fmt.rs
}
}

pub(crate) struct Writer(BufferWriter);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Even though these are private right now we should add some docs to them.

@KodrAus KodrAus merged commit 0cb3860 into master Jan 7, 2018
@KodrAus KodrAus deleted the feat/public-color-api branch January 7, 2018 05:35
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.

1 participant