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.

Sign up for GitHub

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

drop glog from the unquote and parser packages #77

Merged
merged 2 commits into from
Jan 30, 2023
Merged

drop glog from the unquote and parser packages #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31ca894
unquote: drop dependency on glog
mvdan Nov 23, 2022
ec6184f
parser: replace glog Infof calls with an interface
mvdan Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/txtpbfmt/fmt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,16 @@ func main() {
log.Exit(err)
}

// Only pass the verbose logger if its level is enabled.
var logger parser.Logger
if l := log.V(2); l {
logger = l
}
newContent, err := parser.FormatWithConfig(content, parser.Config{
ExpandAllChildren: *expandAllChildren,
SkipAllColons: *skipAllColons,
AllowTripleQuotedStrings: *allowTripleQuotedStrings,
Logger: logger,
})
if err != nil {
errorf("parser.Format for path %v with content %q returned err %v", path, contentForLogging(content), err)
Expand Down
1 change: 1 addition & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0
github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=
github.com/mitchellh/go-wordwrap v1.0.1 h1:TLuKupo69TCn6TQSyGxwI1EblZZEsQ0vMlAFQflz0v0=
github.com/mitchellh/go-wordwrap v1.0.1/go.mod h1:R62XHJLzvMFRBbcrT7m7WgmE1eOyTSsCt+hzestvNj0=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
65 changes: 41 additions & 24 deletions parser/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"strconv"
"strings"

log "github.com/golang/glog"
"github.com/mitchellh/go-wordwrap"
"github.com/protocolbuffers/txtpbfmt/ast"
"github.com/protocolbuffers/txtpbfmt/unquote"
Expand Down Expand Up @@ -78,6 +77,26 @@ type Config struct {

// Use single quotes around strings that contain double but not single quotes.
SmartQuotes bool

// Logger enables logging when it is non-nil.
// If the log messages aren't going to be useful, it's best to leave Logger
// set to nil, as otherwise log messages will be constructed.
Logger Logger
}

func (c *Config) infof(format string, args ...interface{}) {
if c.Logger != nil {
c.Logger.Infof(format, args...)
}
}
func (c *Config) infoLevel() bool {
return c.Logger != nil
}

// Logger is a small glog-like interface.
type Logger interface {
// Infof is used for informative messages, for testing or debugging.
Infof(format string, args ...interface{})
}

// RootName contains a constant that can be used to identify the root of all Nodes.
Expand Down Expand Up @@ -119,7 +138,6 @@ type parser struct {
in []byte
index int
length int
log log.Verbose
// Maps the index of '{' characters on 'in' that have the matching '}' on
// the same line to 'true'.
bracketSameLine map[int]bool
Expand All @@ -144,7 +162,7 @@ func FormatWithConfig(in []byte, c Config) ([]byte, error) {
return nil, err
}
if c.Disable {
log.Infoln("Ignored file with 'disable' comment.")
c.infof("Ignored file with 'disable' comment.")
return in, nil
}
nodes, err := parseWithMetaCommentConfig(in, c)
Expand Down Expand Up @@ -305,9 +323,9 @@ func parseWithMetaCommentConfig(in []byte, c Config) ([]*ast.Node, error) {
if err != nil {
return nil, err
}
if p.log {
Copy link
Member

Choose a reason for hiding this comment

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

Some logs, like this one, have V(2) by default, but others don't, so we need to somehow make the logger differentiate between "default verbosity" and "extra verbosity", such as having a boolean EnableExtraVerbosity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't even notice that there were actually three log levels, not just two. There's Errorf, Infof, and the verbose Infof. Quite confusing to be honest, but I'm also not used to glog :) Naively, I would call the third a Debugf in the interface.

I've taken you up on the extra boolean for the sake of unblocking the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was wrong, there is only one verbosity, the reason for the separate if statement is the performance:

https://pkg.go.dev/github.com/golang/glog#V

The second form is shorter but the first is cheaper if logging is off because it does not evaluate its arguments.

Please delete ExtraVerbosity and instead use something like if p.config.log() implemented as c.Logger.IsVerbose() (c *Config) implemented as return bool(l.Verbose) (l parserLog).

I've checked that this compiles using https://go.dev/play/p/e8pZ220VUGK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we're ending up with this logging interface?

// Logger is a small glog-like interface.
type Logger interface {
	// Infof is used for informative messages, for testing or debugging.
	Infof(format string, args ...interface{})

	// Errorf is used for non-fatal errors, such as invalid meta comments.
	Errorf(format string, args ...interface{})

	// IsVerbose reports whether Infof logs are being printed out.
	IsVerbose() bool
}

Not opposed per se, though the interface stops being "small" or "glog-like" at that point :)

p.log.Infof("p.in: %q", string(p.in))
p.log.Infof("p.length: %v", p.length)
if p.config.infoLevel() {
p.config.infof("p.in: %q", string(p.in))
p.config.infof("p.length: %v", p.length)
}
// Although unnamed nodes aren't strictly allowed, some formats represent a
// list of protos as a list of unnamed top-level nodes.
Expand Down Expand Up @@ -427,7 +445,6 @@ func newParser(in []byte, c Config) (*parser, error) {
in: in,
index: 0,
length: len(in),
log: log.V(2),
bracketSameLine: bracketSameLine,
config: c,
line: 1,
Expand Down Expand Up @@ -560,8 +577,8 @@ func (p *parser) parse(isRoot bool) (result []*ast.Node, endPos ast.Position, er

// Handle blank lines.
if blankLines > 1 {
if p.log {
p.log.Infof("blankLines: %v", blankLines)
if p.config.infoLevel() {
p.config.infof("blankLines: %v", blankLines)
}
comments = append([]string{""}, comments...)
}
Expand Down Expand Up @@ -598,8 +615,8 @@ func (p *parser) parse(isRoot bool) (result []*ast.Node, endPos ast.Position, er
Start: startPos,
PreComments: comments,
}
if p.log {
p.log.Infof("PreComments: %q", strings.Join(nd.PreComments, "\n"))
if p.config.infoLevel() {
p.config.infof("PreComments: %q", strings.Join(nd.PreComments, "\n"))
}

// Skip white-space other than '\n', which is handled below.
Expand Down Expand Up @@ -641,8 +658,8 @@ func (p *parser) parse(isRoot bool) (result []*ast.Node, endPos ast.Position, er
return nil, ast.Position{}, fmt.Errorf("Failed to find a FieldName at %s", p.errorContext())
}
}
if p.log {
p.log.Infof("name: %q", nd.Name)
if p.config.infoLevel() {
p.config.infof("name: %q", nd.Name)
}
// Skip separator.
_, _ = p.skipWhiteSpaceAndReadComments(true /* multiLine */)
Expand Down Expand Up @@ -748,8 +765,8 @@ func (p *parser) parse(isRoot bool) (result []*ast.Node, endPos ast.Position, er
return nil, ast.Position{}, err
}
}
if p.log && p.index < p.length {
p.log.Infof("p.in[p.index]: %q", string(p.in[p.index]))
if p.config.infoLevel() && p.index < p.length {
p.config.infof("p.in[p.index]: %q", string(p.in[p.index]))
}
res = append(res, nd)
}
Expand Down Expand Up @@ -831,10 +848,10 @@ func (p *parser) skipWhiteSpaceAndReadComments(multiLine bool) ([]string, int) {
}
}
sep := p.advance(i)
if p.log {
p.log.Infof("sep: %q\np.index: %v", string(sep), p.index)
if p.config.infoLevel() {
p.config.infof("sep: %q\np.index: %v", string(sep), p.index)
if p.index < p.length {
p.log.Infof("p.in[p.index]: %q", string(p.in[p.index]))
p.config.infof("p.in[p.index]: %q", string(p.in[p.index]))
}
}
return comments, blankLines
Expand Down Expand Up @@ -932,8 +949,8 @@ func (p *parser) readValues() ([]*ast.Value, error) {
vl := p.advance(i)
values = append(values, p.populateValue(vl, preComments))
}
if p.log {
p.log.Infof("values: %v", values)
if p.config.infoLevel() {
p.config.infof("values: %v", values)
}
return values, nil
}
Expand Down Expand Up @@ -966,8 +983,8 @@ func (p *parser) readTripleQuotedString() (*ast.Value, error) {
}

func (p *parser) populateValue(vl string, preComments []string) *ast.Value {
if p.log {
p.log.Infof("value: %q", vl)
if p.config.infoLevel() {
p.config.infof("value: %q", vl)
}
return &ast.Value{
Value: vl,
Expand All @@ -978,8 +995,8 @@ func (p *parser) populateValue(vl string, preComments []string) *ast.Value {

func (p *parser) readInlineComment() string {
inlineComment, _ := p.skipWhiteSpaceAndReadComments(false /* multiLine */)
if p.log {
p.log.Infof("inlineComment: %q", strings.Join(inlineComment, "\n"))
if p.config.infoLevel() {
p.config.infof("inlineComment: %q", strings.Join(inlineComment, "\n"))
}
if len(inlineComment) > 0 {
return inlineComment[0]
Expand Down
5 changes: 2 additions & 3 deletions unquote/unquote_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"strings"
"testing"

log "github.com/golang/glog"
"github.com/kylelemons/godebug/diff"
"github.com/protocolbuffers/txtpbfmt/ast"
)
Expand Down Expand Up @@ -45,8 +44,8 @@ func TestUnquote(t *testing.T) {
continue
}
if diff := diff.Diff(input.want, got); diff != "" {
log.Infof("want: %q", input.want)
log.Infof("got: %q", got)
t.Logf("want: %q", input.want)
t.Logf("got: %q", got)
t.Errorf("Unquote(%v) returned diff (-want, +got):\n%s", input.in, diff)
}

Expand Down