Skip to content
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

Logr 1.1.0 and implement stdr in terms of funcr #18

Merged
merged 6 commits into from
Aug 24, 2021

Conversation

thockin
Copy link
Collaborator

@thockin thockin commented Aug 23, 2021

No description provided.

stdr.go Outdated
depth := l.Formatter.GetDepth() + 2 // offset for this adapter

// ignore errors - what can we really do about them?
if l.std != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to avoid this if check and then perhaps the entire output helper function by ensuring in the constructor that l.std is always non-nil? If NewWithOptions is called with a nil interface, the constructor could create one which calls log.Output.

If l.std continues to be nil in some cases, then GetUnderlying might need a comment about that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Will add a commit

@pohly
Copy link
Contributor

pohly commented Aug 24, 2021

Unrelated to this PR:

stdr/stdr.go

Line 82 in 625208e

// LogCaller tells glogr to add a "caller" key to some or all log lines.

refers to "glogr" instead of "stdr".

The comment about "The glog implementation always logs this information in its per-line header, whether this option is set or not." can be removed, log.New has the flag parameter to control this.

@pohly
Copy link
Contributor

pohly commented Aug 24, 2021

There are no automated tests. Here's one that you can drop into example/example_test.go:

/*
Copyright 2021 The logr Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
	"errors"
	"log"
	"os"

	"github.com/go-logr/stdr"
)

var errSome = errors.New("some error")

func newStdLogger(flags int) stdr.StdLogger {
	return log.New(os.Stdout, "", flags)
}

func ExampleNew() {
	log := stdr.New(newStdLogger(log.Lshortfile))
	log.Info("info message with default options")
	log.Error(errSome, "error message with default options")
	log.Info("invalid key", 42, "answer")
	log.Info("missing value", "answer")
	// Output:
	// example_test.go:35: "level"=0 "msg"="info message with default options"
	// example_test.go:36: "msg"="error message with default options" "error"="some error"
	// example_test.go:37: "level"=0 "msg"="invalid key" "<non-string-key-2>"="answer"
	// example_test.go:38: "level"=0 "msg"="missing value" "answer"="<no-value>"
}

func ExampleWithName() {
	log := stdr.New(newStdLogger(0))
	log.WithName("hello").WithName("world").Info("thanks for the fish")
	// Output:
	// hello/world: "level"=0 "msg"="thanks for the fish"
}

func ExampleNewWithOptions() {
	log := stdr.NewWithOptions(newStdLogger(0), stdr.Options{LogCaller: stdr.All})
	log.Info("with LogCaller=All")
	// Output:
	// "caller"={"file":"example_test.go","line":55} "level"=0 "msg"="with LogCaller=All"
}

@thockin
Copy link
Collaborator Author

thockin commented Aug 24, 2021

Thanks, will add the test

@thockin thockin force-pushed the master branch 2 times, most recently from a95e07b to cef82a3 Compare August 24, 2021 17:40
@@ -84,7 +84,7 @@ type Options struct {
// be treated as zero.
Depth int

// LogCaller tells glogr to add a "caller" key to some or all log lines.
// LogCaller tells stdr to add a "caller" key to some or all log lines.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next sentence can be removed completely ("The glog implementation...").

@thockin
Copy link
Collaborator Author

thockin commented Aug 24, 2021 via email

@thockin
Copy link
Collaborator Author

thockin commented Aug 24, 2021

all glog refs removed

Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pohly pohly merged commit 7e1a3a4 into go-logr:master Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants