Make it easy to define a slightly different format (without writing a formatter from scratch)

[LukasKalbertodt]

Hi there!

While it is possible to define a custom formatting via the Builder, often one only wants to adjust small things. For example, the timestamp is way too noisy for my application, so I want to disable it. But now I have to do everything else by myself. Especially right now I cannot use colors, so I cannot implement the formatting I want. It would be nice to say something like builder.with_timestamps(false). Sure, this would introduce one (or a few, if we want to add options for other parts -- like the module path -- as well) boolean flag that has to be checked for each log message. But that should be fine, right?

[KodrAus]

I agree, it would be nice to be able to tweak the default format without having to start from scratch if all you want to do is disable timestamps, and I don't think a few if statements will make a noticeable difference. I am still just a bit on the fence though, it would be nice for custom formats to be easy enough to use that it's no more trouble to define them than it is to tweak the default :)

Having said that, I'm not against this. I think it'll be nice and easy to discover and use.

[aep]

how about a format string? something like https://github.com/vitiral/strfmt

[LukasKalbertodt]

@aep Format strings have the disadvantage that the programmer can write incorrect syntax. Therefore you'd need to return a Result or Option somewhere which leads to a rather ugly unwrap(). As an alternative, how about a builder pattern again?

format_builder()
    .level()
    .msg()   // a space is added between level and msg automatically
    .str(" [")
    .mod_path()
    .str("]")

// Results in something like:
// INFO this is a log message [test::main]

This could be cooperated into the already existing builder. Internally it would have to build up some kind of simple data structure, like a Vec<Part> where enum Part { Level, ModulePath, Msg, Str(&str), ... }. The speed of that thing needs to be measured, sure. Right now, I think it won't slow things down significantly; there are already quite a few magic fmt calls, at least one heap access and so on.

Sure, a format string might be easier to read; at least for some. I personally dislike most format strings, because one always has to remember a new magic syntax (or rather: look it up every time I need to write a format string).

But this is just an idea I wanted to propose.

[aep]

@LukasKalbertodt builder patterns are nice!

[KodrAus]

Personally, I'm more inclined to simple methods like log_timestamp on a default format builder for enabling/disabling parts of the default format, and any more custom formatting can be done using a custom format like we already support. I think the standard library's formatting macros are a better fit than builders for this because you can get a quick sense of what the format is and most users are already familiar with it.

[LukasKalbertodt]

@KodrAus I'm fine with just a few simple methods, too! So which methods on the builder do we need? How about?

• with_timestamp(bool)
• with_module_path(bool)
• with_level(bool)

I guess disabling the actual log message doesn't make any sense in the general case (and if a user wants it, a custom formatter can solve that for them). Is disabling the level equally useless in most cases?

[KodrAus]

@LukasKalbertodt those all sound good to me 👍 it's another candidate for an environment variable too.

I think it might be a bit weird to disable the level, but maybe if the user is just using env_logger like a fancy println for writing filterable messages then they won't need the level?

For usage, I was thinking it might look something like:

let mut builder = Builder::new();

builder.format(DefaultFormat::new().with_timestamp(false).build());

So we have a specific DefaultFormat builder that outputs the Box<Fn> format on build (too bad implementing fn traits is unstable 😄).

We could put these on the builder directly:

let mut builder = Builder::new();

builder.with_timestamp(false);

My only concern with that is that those methods only make sense when you use the default format, so scoping them to the format they apply to might be clearer than:

let mut builder = Builder::new();

// this doesn't make sense
builder.format(|buf, record| { ... }).with_timestamp(false);

I'm not fully sold either way though, maybe we could solve this with naming, default_format_timestamp or something, and avoid having extra builder types users need to worry about.

What do you think?

[LukasKalbertodt]

I haven't thought about the problem of using both format() and with_timestamp(). But I agree: this would lead to confusion. So I think your idea with DefaultFormat is really nice. Also very easy to extend. People could create an extern crate my_env_logger_formatter in which they just define a formatter that can be passed to .format(). (Not that we necessarily need to promote that; just an idea)

Maybe you could generalize the format() method a bit? Is it possible to add one Into indirection? That way we could avoid the build() call on the DefaultFormatter (and possibly other formatters as well).

[KodrAus]

A generalised format method that doesn't require extra method calls sounds good. I thought we'd run into inference problems with closure arguments by adding indirection, but it looks like we can work around that just fine.

EDIT: Actually we can just use a trait. Not sure why I thought that wouldn't work, it must've been an Into bound I was working with the last time I tried this :)

[LukasKalbertodt]

Sounds good to me!

[KodrAus]

Ah so there is a problem with inferring the types of the closure when we're not explicitly binding a generic to one of the Fn traits :( I expected some compiler errors on the closure arguments, but it's not until we try to use one of those arguments that everything goes pear shaped:

error[E0619]: the type of this value must be known in this context
  --> examples/custom_format.rs:37:25
   |
37 |         let mut style = f.style();
   |                         ^^^^^^^^^

So I think we are back to adding a build method to the default format, or adding a wrapper type around custom formats that implements a Format trait but takes an explicit Fn type, like Custom(|record, f| { ... }).

[LukasKalbertodt]

@KodrAus Is your code online somewhere? Or could you write some minimal example? Would be curious to play with the code. I had hoped this should be possible ...

[KodrAus]

@LukasKalbertodt Sure! I've pushed what I've done to a branch. We can make this work if we add a separate method for the default format, or we require you call .build on it to get a Fn. If you come up with something that works I'd be keen to see it :)

[KodrAus]

So I'm thinking the easiest way forward here is to put the methods on the Builder itself and prefix them with default_format, so we have:

• default_format_timestamp(bool)
• default_format_module_path(bool)
• default_format_level(bool)

It's a bit of a mouthful but it should be clear that if you call .format(...).default_format_timestamp(true) that your custom format isn't going to know anything about the default format flag you set. We can also call this out in the docs.

Does that sound reasonable?

[KodrAus]

The next question is whether default_format_timestamp(bool) et al should clobber a custom format. I would think not, because then you might expect default_format_timestamp to clobber default_format_module_path.

[LukasKalbertodt]

@KodrAus Sorry for the late response :(

I'm not quite in the loop about env_logger releases. Can you still make a breaking change release?

I don't quite like the methods on the Builder directly tbh. IMO it would be really great if we had a format() method and some kind of Formatter trait. Although maybe it's really not worth the extra complexity, I don't quite know.

If breaking changes are still possible, maybe something like the following would be ok?

• Formatter trait, with impl<F> Formatter for F where F: FnMut(...) (maybe a different name since the crate already has a Formatter)
• Builder::format() takes a <F: Formatter>
  • The documentation of format() mentions that -- when you want to pass closures -- you have to specify the argument types. It also includes an example:

  builder.format(|buf: &mut Formatter, record: Record| { ... })

• Maybe add a Builder::format_fn() method which works pretty much like the the format() right now (i.e. takes an FnMut() directly). Thus this method can be used to avoid annotating types manually.

[KodrAus]

I'm not quite in the loop about env_logger releases. Can you still make a breaking change release?

Unfortunately the ship has sailed on 0.5.x, so we'll want to avoid breaking changes for a while :(

After going through the process of upgrading a few crates to the new version I've actually come around to the default_* methods on the builder, even though it doesn't capture the formatting domain as nicely as a trait, because it means importing fewer types and is really straightforward. There's inherent friction against complex logging configuration, so anything we can do to reduce friction is worth it. I also can't think of a case at the moment besides making the default format ergonomic where a trait is more capable than a move closure.

[LukasKalbertodt]

@KodrAus yeah, you're problably right. Then maybe this crate should really go the easy way for now. If the next breaking version bump happens, one can still think about a better way to handle this ^_^

[KodrAus]

This has been rolled in to the 0.5.5 release