Added WithError(err)

[yawn]

This is very small feature request for a function of logrus I use all the time: adding an error message to the context.

[sirupsen]

This is not unreasonable. I'll think a bit about this.

[yawn]

👍

[yawn]

WDYT about implementing Error in *Entry? This could be useful when we'd change the return value of the actual logging methods from void to *Entry, allowing idioms like this:

if err != nil {
  return ctx.WithError(err).Error("This did not work")
}

[yawn]

PR is missing functionality in exported. I'll add that if you like the functionality.

[sirupsen]

@yawn isn't that what you've already done? re adding to *Entry?

I've also decided that this is going in. It should be added to Exported with an accompanying test.

[yawn]

I meant

1. implementing the Error interface in *Entry and
2. returning Entry in Debug, Error etc.

This way we could return errors logged to the context by returning the call to the logging method. I can prepare an example if you want.

[sirupsen]

Yeah, I think that's valid. I've wanted to do that before. Logging is often awkward in Go because it can be unclear when to log and when to return an error, and sometimes you even want to do both—because the context you have is not present. But fewer log lines with more information is often better than a lot of log lines with little information, and makes the code harder to read.

It should be a separate API, but I'm open to the idea.

[sirupsen]

If you could add this to exported I'll get it merged.

[yawn]

I'll do the additions on the train. Regarding the API:

• I think it would not hurt to return the *Entry that log message was created on - currently it's void so, nothing changes IMHO
• Did you have a look at my proposal Proposal: log events #182?

[yawn]

Done, with the proposed API (756db3c) as separate commit.

[yawn]

Do you need anything more from me here @sirupsen?

[aybabtme]

LGTM

[imax9000]

This just broke the build of https://github.com/docker/distribution/tree/master/context

../github.com/docker/distribution/context/http.go:184: cannot use l (type *logrus.Entry) as type Logger in return argument:
    *logrus.Entry does not implement Logger (wrong type for Debug method)
        have Debug(...interface {}) *logrus.Entry
        want Debug(...interface {})

[aybabtme]

@gelraen thanks for letting me know, it seems this interface should be updated:

https://github.com/docker/distribution/blob/master/context/logger.go#L9-L40

The project uses vendored dependencies so it should not be affected by the change. I'll send them a PR anyways.

[imax9000]

@aybabtme please also note that this breaks promise of being drop-in replacement for log package (https://github.com/Sirupsen/logrus/blob/master/README.md#example)

[aybabtme]

It doesn't break the promise for the log.Logger, only if you use the *log.Entry as a log.Logger.

I can't say what the intent was with regard to compatibility, I'll let @sirupsen decide if he sees it as applying to *log.Entry. If so, we can revert the merge.

[yawn]

I'd vote for a partial revert of 756db3c then ("changing the return value"). This is not strictly necessary (except for the cast-to-error usecase) for this PR but is probably useful in any case. It's just seems to break compatibility ...

[sirupsen]

Fortunately this was never released as a version. Thanks for your quick revert @aybabtme.