PR #166 introduced a breaking change in a patch release about logr

[makkes]

@thockin as discussed on Slack.

[thockin]

Pasting (slightly edited) short note from sloack so I don't lose it:

So you can't tell if they intentionally gave you a Discard() or just "don't care". You're right that this change had an unforeseen breaking implication. Sorry about that! I updated the release notes.

We probably don't want to revert that change, so let's think.

Logger is a value and we've (intentionally) made the zero value useful. If you need a caller has to pass something in, I would either make the argument a *Logger (so it can be nil) or ask them to pass Discard() explicitly.

Discard logger already returns false for Enabled(). We could maybe add something like Logger.IsImplemented(), which is only true if you go through New(). I kind of hate it. Open to ideas.

[pohly]

logr.Logger{} != logr.Discard()

I don't think we ever promised that logr.Discard() is or is not equal to the zero value. The API only guaranteed that the Logger value can be compared and that logr.Discard always returns an equal value.

There is a comment that pointer to a logger should be used when detecting "no logger" is important:

logr/logr.go

Lines 130 to 133 in 5d57712

	// Calling methods with the null logger (Logger{}) as instance will crash
	// because it has no LogSink. Therefore this null logger should never be passed
	// around. For cases where passing a logger is optional, a pointer to Logger
	// should be used.

Ignore the comment in that section about the null logger, that is out-dated. But it doesn't change the recommendation.

[thockin]

Oops! We should fix that comment!

…

On Mon, Apr 24, 2023, 12:43 AM Patrick Ohly ***@***.***> wrote: logr.Logger{} != logr.Discard() I don't think we ever promised that logr.Discard() is or is not equal to the zero value. The API only guaranteed that the Logger value can be compared and that logr.Discard always returns an equal value. There is a comment that pointer to a logger should be used when detecting "no logger" is important: https://github.com/go-logr/logr/blob/5d5771278ebb6b08180d7cfd806af158964ebfc1/logr.go#L130-L133 Ignore the comment in that section about the null logger, that is out-dated. But it doesn't change the recommendation. — Reply to this email directly, view it on GitHub <#180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVCWHJYDDBPO2J5YYCTXCYVILANCNFSM6AAAAAAXEIPAUI> . You are receiving this because you were assigned.Message ID: ***@***.***>

[pohly]

-> #182

[thockin]

@makkes - Given that we don't want to revert this - the options are either

a) you use *Logger to indicate optional Logger
b) you don't make it optional, and user says what they mean
c) we add API surface to indicate a difference between Logger{} and Discard(), which I think we don't want to do.

Are we OK to close this?

[makkes]

We already fixed our code and removed the conditional, basically opting for case b) you gave above.

Thanks for the clarification on expectations here, everyone!