parser: swap all log.Errorf calls with error values

[mvdan]

(see commit message)

Updates #70.

[mvdan]

cc @stapelberg

[mvdan]

I somehow broke the tests, sorry. Be right back.

[mvdan]

Should be ready now.

[stapelberg]

The current approach results in a bunch of behavioral changes — what previously was skipped (with an error logged) now aborts the entire formatting, right?

I’m not sure if this is safe to do (cc @kssilveira).

Maybe a more straight-forward change would be to introduce an interface to specify the logger, which we can then hook up to glog internally, but which defaults to the standard library’s log package externally?

[mvdan]

To be clear, that's the path I was taking initially, but it felt wrong to document an API which both returns errors and logs errors. That's why I thought - perhaps we can treat all errors equally by returning.

Fine with me if that's not an acceptable change of behavior, I can close this PR and go back to the original plan. Perhaps a better way to deal with the confusion, then, would be that the logger only deals with Infof and Warnf - then all errors are consistently returned and never logged.

[stapelberg]

(To be clear, I’m not saying this is unacceptable, just saying it needs a careful look by @kssilveira and potentially a global test run.)

---

Maybe the logger can even have just a single method, and it’s up to the caller to decide whether to hook up these messages to info/warning/error leveled logs.

[mvdan]

Maybe the logger can even have just a single method, and it’s up to the caller to decide whether to hook up these messages to info/warning/error leveled logs.

I don't think that's a good idea, given that some of the logs are very noisy and for debugging, and some others are showing ignored errors/warnings.

[Nexucis]

any update on this ? would be cool to have this :) and hopefully seeing this patch in cue v0.5

[mvdan]

@Nexucis cue v0.5 is almost done, so we wouldn't bump a dependency unless we absolutely have to at this point.

This PR does change behavior, and it sounds like the actual maintainers aren't very active, so I'll send a non-breaking PR with my original plan instead. I'm still not a big fan of a library reporting errors in two ways, but it's already doing that in any case :)

[kssilveira]

Thanks for working on this, I am on leave until Jan, sorry for the delay!

On Wed, Nov 23, 2022, 10:16 AM Daniel Martí @.***> wrote:

@Nexucis https://github.com/Nexucis cue v0.5 is almost done, so we wouldn't bump a dependency unless we absolutely have to at this point.

This PR does change behavior, and it sounds like the actual maintainers aren't very active, so I'll send a non-breaking PR with my original plan instead. I'm still not a big fan of a library reporting errors in two ways, but it's already doing that in any case :)

— Reply to this email directly, view it on GitHub https://github.com/protocolbuffers/txtpbfmt/pull/75#issuecomment-1324752272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDCIIS6RCJZGOQQ35KWITWJXOGJANCNFSM6AAAAAAR7RZ3VY . You are receiving this because you were mentioned.Message ID: @.***>

[kssilveira]

I will try this inside Google to see if something breaks and report back.

[kssilveira]

Trying now to see what breaks.

[mvdan]

Since the focus is on https://github.com/protocolbuffers/txtpbfmt/pull/77 now, I assume we should drop this one?

In an ideal world I'd prefer the library to return errors as some form of multi-errors, rather than log them, but the priority is definitely to just remove glog :)

[kssilveira]

I didn't find any test failure caused by this change, so we can do both.

[mvdan]

Ah neat. In that case, if you merge this first, then the logger changes will become a lot easier - I would refactor the other PR a bit.

[mvdan]

@kssilveira it seems like the merge got reverted by https://github.com/protocolbuffers/txtpbfmt/commit/5ecdd59e9a2484af05fdcbcbf63f5614856326e7, is that by design? I can't really refactor the other PR to sit on top of this one if master doesn't include the changes :)

[kssilveira]

Sorry! I am still learning to deal with external contributions, I think I was not supposed to click the merge button here, instead I need to merge it internally first and then the robot will mark it as merged.

Could you make a pull request again? I will try to properly accept it this time.

[mvdan]

Ah, no worries. Master is causing conflicts in the meantime, unfortunately, so I'll try to solve those.

[mvdan]

Done in https://github.com/protocolbuffers/txtpbfmt/pull/84.