parser: swap all log.Errorf calls with error values #75
Conversation
|
cc @stapelberg |
|
I somehow broke the tests, sorry. Be right back. |
As I was working on my patch for protocolbuffers#70, I was wondering if we really needed a logger interface with both Infof and Errorf. After all, a library should report its errors as values, not via a logger. Turns out it's relatively straightforward to do so. All functions that used to call log.Errorf are themselves called from top-level APIs which are able to return an error, so we just need to thread them. One special case is getMetaCommentStringValues, which used to report each error and continue rather than stop. We currently do not have an idiomatic way to report multiple errors at once, so return the first error for now. In the future, we could use https://go.dev/issue/53435. Updates protocolbuffers#70.
mvdan
force-pushed
the
parser-log-errorf
branch
from
218d9bf to
4454a9b
Compare
|
Should be ready now. |
|
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? |
|
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 |
|
(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. |
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. |
|
any update on this ? would be cool to have this :) and hopefully seeing this patch in cue v0.5 |
|
@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 :) |
|
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
<#75 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACDCIIS6RCJZGOQQ35KWITWJXOGJANCNFSM6AAAAAAR7RZ3VY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I will try this inside Google to see if something breaks and report back. |
|
Trying now to see what breaks. |
|
Since the focus is on #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 :) |
|
I didn't find any test failure caused by this change, so we can do both. |
|
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. |
|
@kssilveira it seems like the merge got reverted by 5ecdd59, 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 :) |
|
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. |
|
Ah, no worries. Master is causing conflicts in the meantime, unfortunately, so I'll try to solve those. |
|
Done in #84. |
(see commit message)
Updates #70.