Add DisableEscapeHTML option for json logger

[soh335]

Hi.
I add DisableEscapeHTML option for json logger. If pass log.DisableEscapeHTML(true) to NewJSONLogger, we can get more readability.

Thanks.

[soh335]

oops. sorry I break func(io.Writer) log.Logger interface.

I think there are some ideas.

• add like a NewJSONLoggerWithOption ( other constructor function dont contain WithOption. it seems to be bad)
• create Option interface and define constructor function as func(w: io.Writer, options ...Options) ( need to change other loggers )
• call SetHTMLEscape(false) in jsonLogger without option. ( breaking change )
• other idea ?

[ChrisHines]

My initial thought is to call SetHTMLEscape(false) in jsonLogger without option. I'm not sure how much that is really a breaking change. The output will still be JSON compliant. I wouldn't consider the browser as a primary target for the output of jsonLogger.

[soh335]

I wouldn't consider the browser as a primary target for the output of jsonLogger.

I think so too. so no one care about this change, call SetHTMLEscape(false) in jsonLogger seems to be better than others.

[nicolaiskogheim]

enc.SetEscapeHTML(!l.disableEscapeHTML) (log/json_logger.go:50)

This would read better if the property was named escapeHTML instead, I think.

[soh335]

@peterbourgon how about this ?

[peterbourgon]

This should be

escapeHTML bool

defaulting to false.

[peterbourgon]

This should be

func SetEscapeHTML(on bool) JSONLoggerOption {

mirroring json/Encoder.SetEscapeHTML.

[peterbourgon]

This should be

enc.SetEscapeHTML(l.escapeHTML)

[soh335]

@peterbourgon thanks for reviewing and change to SetEscapeHTML.
/log/level/level_test.go and log/term/example_test.go is failed because log.NewJSONLogger is not func(io.Writer) log.Logger interface.

Should I change like this ? (I think its not smart...)

-                       format: log.NewJSONLogger,
+                       format: func(w io.Writer) log.Logger { return log.NewJSONLogger(w) },

-       logger := term.NewLogger(os.Stdout, log.NewJSONLogger, colorFn)
+       logger := term.NewLogger(os.Stdout, func(w io.Writer) log.Logger { return log.NewJSONLogger(w) }, colorFn)

[peterbourgon]

Seems like an OK solution.

[ChrisHines]

I'm not convinced yet. There is something very satisfying about the functions signatures of log.NewLogfmtLogger and log.NewJSONLogger as they are today. How important is it to allow the JSONLogger to be configurable on this dimension?

[peterbourgon]

I could also see this being solved with e.g.

type HTMLEscapingLogger struct {
    next log.Logger
}

func (l HTMLEscapingLogger) Log(keyvals ...interface{}) error {
    for i := 0; i < len(keyvals); i += 2 {
        keyvals[i+1] = htmlEscape(keyvals[i+1])
    }
    l.next(keyvals...)
}

[soh335]

Is it mean call SetEscapeHTML(false) in json logger and call like html.EscapeString(v) function in HTMLEscapeLogger ?
I want to just call enc.SetEscapeHTML (its simple), But I know keep interface is also important...

[soh335]

@ChrisHines @peterbourgon it seems to be difficult both to keep func(io.Writer) log.Logger and add logger options currently. And if dont allow to call enc.SetEscapeHTML without option in json logger, maybe this pr cant be merged. so I will close.

[ChrisHines]

I haven't seen any objections to just changing the existing JSONLogger to always call enc.SetEscapeHTML(false) and leave the exported API untouched. I would be OK with that change.