UP032: Don't suggest f-string inside logging statements

[rpdelaney]

Keywords: UP302, G004

$ ruff --version
ruff 0.6.9

Consider this statement. UP032 objects to the use of str.format here:

logger.info("Result received: {}".format(result))

Following its suggestion to use an f-string instead, one might try this:

logger.info(f"Result received: {result}")

However, the recommendation to use an f-string is inappropriate, as this code now throws G004.

Better is:

logger.info("Result received: %s", result)

[MichaReiser]

Ruff should suggest you two violations for your code example if you have both UP032 and G001 enabled (playground)

logger.info("Result received: {}".format(result))

1. that the there's a str.format call in a logging call and 2. that the string could be replaced with an f-string

I do see how, ideally, Ruff would only flag 1. because fixing 1. also address 2. However, it introduces some complexity. E.g should 2. be raised if the user suppressed 1. with a noqa comment? That's why we avoid adding rule logic rely on other rules, in addition to that doing so increases the complexity of rule implementations (and that makes ruff slower).

That G004 flags the f-string is correct and consistent to the behavior with UP032 and G001.

[rpdelaney]

That G004 flags the f-string is correct

Fully agree.

As a user, I want the linter to give me the shortest path to resolution. Resolving the UP032 finding by following ruff's suggestion, only to get another finding, is not ideal.

Perhaps it would be enough if UP032 checks for a logging context, and if found, offers the solution from G004 instead?

[MichaReiser]

Perhaps it would be enough if UP032 checks for a logging context, and if found, offers the solution from G004 instead?

That would be possible but we prefer to avoid it because rules then become very interdependent. E.g. it should only do so when G004 is enabled, otherwise you still want to see the errors.

I would have to look at the fixability of those rules but ruff check --fix iterates until there are no new fixable violations.

[MichaReiser]

Maybe I'm overly strict here. @AlexWaygood what's your take on whether we should make UP032 logger aware (but only if G004 is enabled?)

[rpdelaney]

but only if G004 is enabled?

I am nearing the limits of my knowledge, and therefore, my confidence. That said, I'll provide an opposing argument for your consideration. The current behavior is that UP032 recommends an f-string in this case: a recommendation that is categorically wrong. Although the user might not have selected G004, that doesn't support giving wrong advice about how to resolve UP032.

[MichaReiser]

I looked at how we detect logging calls, and the implementation is prone to false positives because of Ruff's lack of multifile analysis (we're working on it). Ignoring UP032 in logging statements would also disable the rule for some call sites where it isn't an actual logging call. This is less of a problem for G004 because you would notice that it isn't a logging call and add a noqa. However, you would then want the linter to raise G004 because this string should now be rewritten as an f-string.

Although the user might not have selected G004, that doesn't support giving wrong advice about how to resolve UP032.

A user that disabled G004 and enabled UP032 decided that string formatting in logging calls is okay for them. Whatever the reasons. But they do care that al code uses f-string formatting. I would be surprised if my linter didn't flag UP032 in this case because using an f-string is the "correct" fix given my preference.

Either way, I think the current behavior isn't unreasonable, and suppressing the error would lead to too many false negatives because our "is logging call" method has a fair number of false positives. I'll wait to hear @AlexWaygood thoughts but I'm for closing this issue given these considerations.

[tdulcet]

Maybe another solution to this would be to add an autofix for the G001, G002 and G004 rules. This would effectively perform the reverse of pyupgrade and convert the printf, str.format and f-string syntax to use the logger's builtin formatter, which is of course just the printf style without the % operator. That would be compatible with the UP031 and UP032 rules, and it should not matter what order the autofixes were performed, as it would eventually produce the same result and resolve all of the lint errors.

[rpdelaney]

I find the responses from Micha are reasonable and persuasive, so I'm withdrawing this feature request. If anyone else wants to re-open discussion they can create another issue, discussion thread, or whatever is appropriate at that time.