New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define and implement missing impls for Logger #7

Closed
sebasmagri opened this Issue Aug 19, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@sebasmagri
Owner

sebasmagri commented Aug 19, 2017

Logger doesn't currently have any impls. We should define which ones are needed and implement them.

@sebasmagri sebasmagri referenced this issue Aug 19, 2017

Closed

API Guidelines tracking issue #5

9 of 9 tasks complete

@KodrAus KodrAus added the help wanted label Sep 12, 2017

@jimmycuadra

This comment has been minimized.

Show comment
Hide comment
@jimmycuadra

jimmycuadra Oct 12, 2017

Contributor

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. :}

Contributor

jimmycuadra commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus Oct 18, 2017

Collaborator

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

Are we happy to close this one?

Collaborator

KodrAus commented Oct 18, 2017

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

Are we happy to close this one?

@jimmycuadra

This comment has been minimized.

Show comment
Hide comment
@jimmycuadra

jimmycuadra Oct 18, 2017

Contributor

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.

Contributor

jimmycuadra commented Oct 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus Oct 18, 2017

Collaborator

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?

Collaborator

KodrAus commented Oct 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jimmycuadra

jimmycuadra Oct 18, 2017

Contributor

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.

Contributor

jimmycuadra commented Oct 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Oct 18, 2017

Collaborator

What's the proposed log change?

Collaborator

sfackler commented Oct 18, 2017

What's the proposed log change?

@KodrAus

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus Oct 18, 2017

Collaborator

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

Collaborator

KodrAus commented Oct 18, 2017

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

@KodrAus

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus Oct 21, 2017

Collaborator

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.

Collaborator

KodrAus commented Oct 21, 2017

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 KodrAus added this to the 0.3.x milestone Oct 23, 2017

@KodrAus

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus Oct 30, 2017

Collaborator

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.

Collaborator

KodrAus commented Oct 30, 2017

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.

@KodrAus KodrAus closed this Oct 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment