logrusr logger #2
Conversation
|
@JeremyMarshall hey, thanks for the pull request! I just saw this, sorry for the delay. I'll try to take a look ASAP and get it merged in. More to come. |
There was a problem hiding this comment.
Hi @JeremyMarshall, thanks again for the pull request! Sorry it's taken me almost a year to get around to this. I'd love to get your Logrus implementation merged in. Can you take a look at my requested changes when you get a chance?
README.md
Outdated
| [logfmt](http://godoc.org/github.com/kr/logfmt). | ||
| | Implementation | Notes | | ||
| |---|---| | ||
| |[stdlogr](stdlogr)| A *standard* implementation () using `fmt` that prints logs to `STDOUT` in [logfmt](http://godoc.org/github.com/kr/logfmt)| |
There was a problem hiding this comment.
Please remove the () after "implementation".
go.mod
Outdated
| @@ -1,3 +1,7 @@ | |||
| module github.com/activeshadow/logr | |||
| module github.com/JeremyMarshall/logr | |||
There was a problem hiding this comment.
This file (go.mod) needs to be reverted back so the module name remains "github.com/activeshadow/logr". You can use your version w/ Logrus until this is merged by using the replace directive in go.mod.
logrusr/logger.go
Outdated
| "request": &logrus.Fields{ | ||
| "name": this.name, | ||
| "kvs": kvs, | ||
| "this_kvs": this.kvs, |
There was a problem hiding this comment.
Would it make sense to merge key/value pairs? You combine them in the WithValues function below, so why not here too?
There was a problem hiding this comment.
I think you're right here. From memory, I think this.kvs is added when you create the logger and kvs for this particular call but they can be merged.
What about cases where the maps share keys?
There was a problem hiding this comment.
In cases where the maps share keys, I would think the most recent key should win. So here, the key passed to the Info function would overwrite the key created when the logger was created.
logrusr/logger.go
Outdated
| "error": err, | ||
| "name": this.name, | ||
| "kvs": kvs, | ||
| "this_kvs": this.kvs, |
There was a problem hiding this comment.
Would it make sense to merge key/value pairs? You combine them in the WithValues function below, so why not here too?
logrusr/logger.go
Outdated
| }, | ||
| }) | ||
|
|
||
| if logger != nil { |
There was a problem hiding this comment.
I don't see a path in the Logrus code where this would ever be nil.
logrusr/logger.go
Outdated
| }, | ||
| }) | ||
|
|
||
| if logger != nil { |
There was a problem hiding this comment.
I don't see a path in the Logrus code where this would ever be nil.
logrusr/logger_test.go
Outdated
| vLogger := logger.V(1) | ||
| vLogger.(*LogrusInfoLogr).clock = &clock{mock: mock} | ||
| vLogger.Info("test verbose log", "hello", "crazy world") | ||
| } |
There was a problem hiding this comment.
Can you either validate the result of each test function above with t.Fail calls or turn them into examples (instead of tests) with expected output comments?
|
@activeshadow Let me take a look. It's been a while. I am using my fork in a kubebuilder pet project though |
remove time property
|
Hopefully better now. I converted the tests to gomega/ginkgo |
There was a problem hiding this comment.
@JeremyMarshall looks good! Just a few more small requests and I'll be comfortable merging. Thanks!
logrusr/logger.go
Outdated
| @@ -5,7 +5,7 @@ import ( | |||
| "github.com/activeshadow/logr/util" | |||
| "github.com/go-logr/logr" | |||
| "github.com/sirupsen/logrus" | |||
| "time" | |||
| // "time" | |||
There was a problem hiding this comment.
Can you remove this commented require directive?
| ret[extra[i].(string)] = extra[i+1] | ||
| } | ||
| return ret | ||
| } |
There was a problem hiding this comment.
I think this function could be simplified, and a range operation could be avoided.
for i := 0; i < len(extra); i += 2 {
kv[extra[i].(string)] = extra[i+1]
}
return kv
I put together a logrus logger, just in case.
kubebuilderuses logr, which may not be everyones ideal logger