drop glog from the unquote and parser packages #77

mvdan · 2022-11-23T13:31:54Z

(see commit messages - please do not squash)

Fixes #70.

mvdan · 2022-11-23T13:33:23Z

This is an alternative to #75. Note that, unlike the other patch, this avoids any breaking changes where possible.

There is one minor breaking change. Any parser users which did want glog should now pass it along explicitly, just like I'm doing in cmd/txtpbfmt. It can't be kept there by default, because we want to drop the hard dependency in the first place.

kssilveira · 2022-11-29T15:21:46Z

I think this will work, I will comment on the diff.

kssilveira · 2022-11-29T15:24:30Z

parser/parser.go

@@ -163,7 +173,9 @@ func Format(in []byte) ([]byte, error) {
 func FormatWithConfig(in []byte, c Config) ([]byte, error) {
 	addMetaCommentsToConfig(in, &c)
 	if c.Disable {
-		log.Infoln("Ignored file with 'disable' comment.")
+		if c.Logger != nil {


Can we avoid checking for nil in every use, and instead make every public function set logger to an internal noopLogger (that doesn't do anything) if logger is nil?

There's no good place to have such a read-write field to set to a noopLogger. The parser type isn't enough, because we need to log from some funcs which don't hang off the parser.

I assume you didn't like the repetition, so I set up two methods on the config type to deduplicate the nil checks. I don't think the nil checks harm performance, because an interface call always has to check whether the interface is nil or not.

kssilveira · 2022-11-29T15:25:53Z

parser/parser.go

@@ -322,9 +334,9 @@ func parseWithMetaCommentConfig(in []byte, c Config) ([]*ast.Node, error) {
 	if err != nil {
 		return nil, err
 	}
-	if p.log {


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.

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.

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.

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 :)

kssilveira

I wanted to approve only one commit, not the whole pull request, sorry.

kssilveira · 2023-01-04T10:07:48Z

parser/parser.go

@@ -322,9 +334,9 @@ func parseWithMetaCommentConfig(in []byte, c Config) ([]*ast.Node, error) {
 	if err != nil {
 		return nil, err
 	}
-	if p.log {


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.

kssilveira · 2023-01-06T06:19:55Z

I think this is what we need. The difference to glog is the bool type, which I think we can't do for an interface.

…

On Thu, Jan 5, 2023, 9:31 AM Daniel Martí ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In parser/parser.go <#77 (comment)> : > @@ -322,9 +334,9 @@ func parseWithMetaCommentConfig(in []byte, c Config) ([]*ast.Node, error) { if err != nil { return nil, err } - if p.log { 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 :) — Reply to this email directly, view it on GitHub <#77 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACDCIN2YEEHXZRVFZVIQ53WQ2BGXANCNFSM6AAAAAASI7A4C4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mvdan · 2023-01-07T13:26:41Z

Should be ready now. I went for InfoLevel() bool rather than IsVerbose() bool, to clarify that it returns whether the info level at Infof is enabled or not. The notion of "verbose" seemed confusing to me, as it could mean other things such as how detailed the logs should be, rather than whether they are printed or not.

We can log straight into testing.T.

Now that glog.Errorf calls are returned as error values, we can replace glog.Verbose.Infof calls with an interface. All calls go through a logger in the config. By default the logger is nil, which disables logging, so we have nil checks when logging. Note that the library uses a nil check on the logger to tell whether or not it can avoid constructing the log messages. We don't need a method like "IsInfoLevel" or "IsEnabled", since the interface only knows how to log at the info level. With that in mind, keep the behavior in cmd/txtpbfmt unchanged; it now passes log.V(2) in the config if its level is enabled. Fixes protocolbuffers#70.

mvdan · 2023-01-23T11:32:49Z

Since the Errorf PR was merged (thanks @kssilveira!), I've now rebased and updated this PR accordingly.

The Errorf method is gone, as it's no longer needed. And I've also removed InfoLevel - since we now only need a single logger method, we can check that the logger is non-nil to construct log messages.

mvdan · 2023-01-23T11:34:08Z

We could also go further and have Logger be just a func signature type, but I think that could be a bit limiting in the future.

kssilveira · 2023-01-24T10:31:40Z

I will accept this, thanks!

mvdan · 2023-01-26T14:56:04Z

Excellent, let me know if I can help to get this merged :)

mvdan · 2023-01-30T09:58:54Z

FYI @kssilveira @stapelberg, this was merged with a change from interface{} to any - which was never in my PR, as far as I can tell. That broke the build, as any requires go 1.18 or later in your go.mod. Master's build appears to be broken for now.

kssilveira · 2023-01-30T10:04:05Z

Yes, sorry, fixing.

kssilveira reviewed Nov 29, 2022

View reviewed changes

kssilveira approved these changes Nov 29, 2022

View reviewed changes

kssilveira requested changes Nov 29, 2022

View reviewed changes

mvdan force-pushed the parser-glog branch from 628c6c5 to a963b81 Compare November 29, 2022 16:58

kssilveira requested changes Jan 4, 2023

View reviewed changes

mvdan force-pushed the parser-glog branch from a963b81 to d8aa537 Compare January 7, 2023 13:25

mvdan mentioned this pull request Jan 7, 2023

parser: swap all log.Errorf calls with error values #75

Merged

mvdan added 2 commits January 23, 2023 11:12

unquote: drop dependency on glog

31ca894

We can log straight into testing.T.

mvdan force-pushed the parser-glog branch from d8aa537 to ec6184f Compare January 23, 2023 11:31

copybara-service bot merged commit 765cbae into protocolbuffers:master Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop glog from the unquote and parser packages #77

drop glog from the unquote and parser packages #77

mvdan commented Nov 23, 2022

mvdan commented Nov 23, 2022

kssilveira commented Nov 29, 2022

kssilveira Nov 29, 2022

mvdan Nov 29, 2022

kssilveira Nov 29, 2022

mvdan Nov 29, 2022

kssilveira Jan 4, 2023

mvdan Jan 5, 2023

kssilveira left a comment

kssilveira Jan 4, 2023

kssilveira commented Jan 6, 2023 via email

mvdan commented Jan 7, 2023

mvdan commented Jan 23, 2023

mvdan commented Jan 23, 2023

kssilveira commented Jan 24, 2023

mvdan commented Jan 26, 2023

mvdan commented Jan 30, 2023

kssilveira commented Jan 30, 2023

drop glog from the unquote and parser packages #77

drop glog from the unquote and parser packages #77

Conversation

mvdan commented Nov 23, 2022

mvdan commented Nov 23, 2022

kssilveira commented Nov 29, 2022

kssilveira Nov 29, 2022

Choose a reason for hiding this comment

mvdan Nov 29, 2022

Choose a reason for hiding this comment

kssilveira Nov 29, 2022

Choose a reason for hiding this comment

mvdan Nov 29, 2022

Choose a reason for hiding this comment

kssilveira Jan 4, 2023

Choose a reason for hiding this comment

mvdan Jan 5, 2023

Choose a reason for hiding this comment

kssilveira left a comment

Choose a reason for hiding this comment

kssilveira Jan 4, 2023

Choose a reason for hiding this comment

kssilveira commented Jan 6, 2023 via email

mvdan commented Jan 7, 2023

mvdan commented Jan 23, 2023

mvdan commented Jan 23, 2023

kssilveira commented Jan 24, 2023

mvdan commented Jan 26, 2023

mvdan commented Jan 30, 2023

kssilveira commented Jan 30, 2023