Define and implement missing impls for Logger about env_logger

[jimmycuadra]

I'd be interested in implementing this once it's decided.

I took a quick look to see if there were any common traits (e.g. Clone, Hash) that are easy to derive with the current code. I don't think any of them can be derived for Logger because of the format field which is Box<Fn(&mut Write, &Record) -> io::Result<()> + Sync + Send>. I believe it'd need to use a generic type in order to specify that type in the box is Clone, Hash, etc.

In any case, I'm not sure there's specific use case for Logger and/or Builder implementing any of these common traits. But I thought it couldn't hurt to see if we could get them for free. :}

[KodrAus]

I think you're right @jimmycuadra. You don't usually interact with the Logger directly anyways.

Are we happy to close this one?

[jimmycuadra]

There is one question to follow my previous comment: Why is the format field specified the way that it is instead of using a generic type? There must be some history here that I don't know, and that comes from the log crate itself and not env_logger. Depending on the reason it was done this way, maybe it would be worth considering using a generic type instead so more traits like Clone could be implemented when the concrete type does as well.

[KodrAus]

Oh right. I think it's a trait object to keep the signature of Logger simpler. I don't think we could make it generic unless we also offer a simple way to get the filtering for custom loggers built off env_logger without having to juggle generics. But having separate filtering for custom loggers and a generic Logger type is something I'd totally be on board with.

If we were going to make it generic then I think we should introduce a trait for formatting and blanket implement it for closures along with a well-typed default implementation, maybe even making it a default parameter on Logger, something like:

trait LogFormat {
    fn fmt(&self, record: Record, w: &mut Write);
}

struct DefaultFormat;

impl LogFormat for DefaultFormat { ... }

impl<F> LogFormat for F where F: Fn(&mut Write, Record) { ... }

struct Logger<F = DefaultFormat> { ... }

What do you think?

[jimmycuadra]

That looks good to me. Is this something we should bring up on the log crate's issue tracker? AFAIK they're ready to ship another version of the crate and there isn't any additional design work planned, but I haven't been involved up to this point so I don't know whether this is worth bringing up or trying to change.

[sfackler]

What's the proposed log change?

[KodrAus]

@sfackler there's no proposed change to log as far as I'm aware. Just env_logger::Logger.

[KodrAus]

I spent some time playing around with this and I don't think a generic type is going to work out on the Logger here. The main issue is that we can't change that generic type in the builder without returning a new value, but since we're using &mut self receivers that doesn't work. There are also some inference issues with the blanket impl for Fn on a formatting trait, which is a bit unfortunate but makes sense.

So I think it was worth exploring, but it doesn't seem like a big enough win to cause a lot of churn over. I would still like us to have some alternative way to build your own logger using env_logger's filter parsing that doesn't mean we need a Silent variant on Target though.

[KodrAus]

I'm going to go ahead and close this, leaving the current set of trait impls as-is.

Please let me know if there's more you think we should do here.