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

Please remove the hard dependency on glog #70

Closed mvdan closed 1 year ago

mvdan commented 1 year ago

The parser imports https://github.com/golang/glog directly, for calls such as:

https://github.com/protocolbuffers/txtpbfmt/blob/fc78c767cd6a4e6e3953f5d72f1e0e4c5811990b/parser/parser.go#L392 https://github.com/protocolbuffers/txtpbfmt/blob/fc78c767cd6a4e6e3953f5d72f1e0e4c5811990b/parser/parser.go#L989

It's understandable to want to have some verbose logging, but I think it's a bad idea for a "pure" Go library such as a parser to make direct calls to a logging library:

I think the best and simplest API fix would be to declare a logging interface and use it, such as:

type Logger interface {
    Infof(format string, args ...interface{}) // used for verbose logs
    Errorf(format string, args ...interface{})
}

One could also argue that error logs should be returned as error values instead, but I'm not very familiar with how the glog API is designed in detail.

I'm filing this because I'm the maintainer of a relatively large open source program where we use and ipmort txtpbfmt, but we pull glog indirectly too, which unfortunately affects us in multiple ways (e.g. https://github.com/cue-lang/cue/issues/1199, but also binary size etc).

stapelberg commented 1 year ago

Hey Daniel! 👋

Agreed on all points. For historical context: we needed to get a number of ducks in a row for the initial open-sourcing of txtpbfmt, and doing something better than just swapping our internal log package for glog wasn’t in the cards back then.

Would you be willing to send a pull request which implements the interface you suggested? cc me on it, and we can get this improved.

Thanks

mvdan commented 1 year ago

Oh, I wasn't aware you were involved :) I can certainly send a quick PR.

Nexucis commented 1 year ago

Hi,

I was wondering if there is an on going fix. As a cue user, I was quite annoyed to see the glog flags poping in my binary. If there is none, perhaps I could give a try.

stapelberg commented 1 year ago

Sending the PR is the fix. AFAICT, there was no PR yet

mvdan commented 1 year ago

Thanks for the pings - I completely forgot about this. I can send the PR today.

Nexucis commented 1 year ago

ooh I was going to do it as well haha. Well thank you for doing it @mvdan !