PR #166 introduced a breaking change in a patch release

[makkes]

We (the Flux team) ran into a bug related to this PR. From our perspective the PR is breaking the assumption that logr.Logger{} != logr.Discard() and it actually led to a bad bug experienced by some of our users.

Since the change was introduced as part of a logr patch release (1.2.4) I wanted to let you know (and discuss) that this might be pretty unexpected given it's "just" a patch release and not a minor or major release and that the logr project states to be strict about following semver.

Don't get me wrong, please. I think the change introduced in #166 is very good, it's just the unfortunate inclusion of it in a patch release that might lead to unforseen bugs in dependent code.

I'm happy to hear your people's opinion on this.

[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!