protocolbuffers / txtpbfmt

txtpbfmt parses, edits and formats text proto files in a way that preserves comments.
Apache License 2.0
96 stars 19 forks source link

drop glog from the unquote and parser packages #77

Closed mvdan closed 1 year ago

mvdan commented 1 year ago

(see commit messages - please do not squash)

Fixes #70.

cc @stapelberg @kssilveira

mvdan commented 1 year ago

This is an alternative to https://github.com/protocolbuffers/txtpbfmt/pull/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 commented 1 year ago

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

kssilveira commented 1 year ago

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 https://github.com/protocolbuffers/txtpbfmt/pull/77#discussion_r1062219168 :

@@ -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 https://github.com/protocolbuffers/txtpbfmt/pull/77#discussion_r1062219168, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDCIN2YEEHXZRVFZVIQ53WQ2BGXANCNFSM6AAAAAASI7A4C4 . You are receiving this because you were mentioned.Message ID: @.***>

mvdan commented 1 year ago

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.

mvdan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I will accept this, thanks!

mvdan commented 1 year ago

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

mvdan commented 1 year ago

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 commented 1 year ago

Yes, sorry, fixing.