False positive consider-using-f-string for localization string

[earonesty]

Current problem

just got a bunch of "consider-using-f-string".

Desired solution

f-strings basically defeats localization. the idea that f-strings are the "right thing to do all the time" is wrong. doesn't belong in a linter.

Additional context

yeah, i'm sure some people argued about this in a pep... but this is the first time the rest of the world bumped into it. f-strings are only "more readable" when there are many parameters, and even then .... not so much:

some_big_long_string.format(date=local_date, time=local_time, username=current_user)

raise UserFacingException(localize("An error in %{program_name} occured"))

why pylint these?

[DanielNoord]

I would like the opinion of @Pierre-Sassoulas and @cdce8p on this.

Personally, since f-strings give a performance improvement over format() or % I believe suggesting these should be part of a linter. Besides the performance improvement I like how I can know what's included in a string immediately without looking at the end of the string for the arguments to format() or %. However, maintainers of pylint had their own opinions about this and I know Marc has disabled it in his own environment. This difference in opinion is also why the checker is placed under RecommendationChecker.

[cdce8p]

As @DanielNoord mentioned, I've disabled the check for me personally.

I use f-strings wherever I can, however there are some situations in which format() is just more readable. My concern was that pylint isn't able to sufficiently distinguish between a "good" and "bad" case. There might be a small performance benefit for f-strings, but that isn't enough to sacrifice readability, at least not for me.

I can acknowledge that for some this check might be quite helpful. Maybe moving it to an optional checker would resolve that situation. That would fit together nicely with #4787 possible-forgotten-f-prefix.

[Pierre-Sassoulas]

f-strings are only "more readable"

First, let's be very clear that opinions about what is more readable are not the main reason why we added the checker as default. It's mainly because of provable major relative speed disparities. You're incurring a 3* time of processing when using format. We have had a similar warning for logger lazy formatting for a long time and for a smaller performance gain, so this warning is not out of character for pylint. If you're localizing or have a lot of template in your project, it's possible to disable this in pylintrc.

Also, it's possible to reasonably deal with long string using other tricks that do not impact performance, for example using triple quotes:

my_long_str = f"""
Some big long string format with a date:
{local_date} in {local_time}
"""

Or multiple line:

my_long_str = (
    "Some big long string format with a date:\n"
    f"{local_date} in {local_time}"
)

Now, I agree there is a false positive for format template but the idea behind still shipping it was that templating is not used so often that it's unreasonable to disable this warning on template (we did it for pylint we had maybe 5 false positives on 20ksloc). We deal with a similar issue with possible-forgotten-f-prefix, but in the case of possible-forgotten-f-prefix we're dealing with a subset of string containing {var} and not all the strings like for consider-using-f-string . So the probability that it's a legit template and not a mistake where f was forgotten is a lot higher thus we're not shipping it.

We have a hard time distinguishing legit template strings that are formatted to be used later with format elsewhere ie "{date}, {time} {username}" vs f"{local_date}, {local_time} {current_user}" where date time and username can be variable existing in the scope or not. Distinguishing between the two reliably would reduce the number of false positive for consider-using-f-string but it's a work in progress and probably impossible when the template variables exist in the scope where the template is created.

[DanielNoord]

As I'm the original author of this checker I am happy to take this one, but I need some more info about the false positive. What is raising a message that shouldn't raise a message?

[Pierre-Sassoulas]

For me, false positive for consider-using-f-string are the same one than for possible-forgotten-f-prefix, ie it's the string supposed to be used with format later on like server_template = "Server: {server}" used later with server_template .format(server=current_server)" and that would not work as f-strings. (Discussed in detail here : #4787).

[DanielNoord]

Yes, but we don't emit consider-using-f-string on those. We only emit it when format() or % is used on a string defined in place. ie.:

a = "string {}".format(1) # consider-using-f-string
b = "template string {}"
b.format(1)

************* Module test
test.py:1:4: C0209: Formatting a regular string which could be a f-string (consider-using-f-string)

------------------------------------------------------------------
Your code has been rated at 6.67/10 (previous run: 0.00/10, +6.67)

So defining template strings and using format later shouldn't be a problem.

[Pierre-Sassoulas]

My bad, I should have remembered that. So in my opinion the only false positive is for localization with gettext, something like:

n = len(os.listdir('.'))
cat = GNUTranslations(somefile)
message = cat.ngettext(
    'There is %(num)d file in this directory',
    'There are %(num)d files in this directory',
    n) % {'num': n}

[DanielNoord]

❯ cat test.py
import os
from gettext import GNUTranslations

n = len(os.listdir("."))
cat = GNUTranslations(somefile)
message = cat.ngettext(
    "There is %(num)d file in this directory", "There are %(num)d files in this directory", n
) % {"num": n}
❯ pylint '***/pylint/test.py'
************* Module test
test.py:5:22: E0602: Undefined variable 'somefile' (undefined-variable)

------------------------------------------------------------------
Your code has been rated at 0.00/10 (previous run: 0.00/10, +0.00)

Doesn't seem to issue a message 😄

[sergeyklay]

Well, I think pylint should be a bit more smart

W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)

_ctx.logger.info(f'Processes to be spawned: {jobs}')

W1309: Using an f-string that does not have any interpolated variables (f-string-without-interpolation)

_ctx.logger.info(f'Processes to be spawned: %s' % jobs)

It looks like some kind of trolling.

[DanielNoord]

This is indeed a false positive. Sorry about that! Going to work on a fix now!

Edit: just saw that this is not based on consider-using-f-string. Or am I misunderstanding your example?

[DudeNr33]

Am I missing something, or aren't both examples provided by @sergeyklay entirely valid pylint messages?
Second example should just drop the f-string prefix to satisfy pylint and best practices.

[DanielNoord]

I think pylint might complain about it not being an f-string because of consider-using-f-string and keeps complaining when you do because then the logger messages get triggered.

[Pierre-Sassoulas]

The right way if you want to use lazy formatting so the string formatting is done only if necessary is:

_ctx.logger.info('Processes to be spawned: %s', jobs)

If you want to use f-string in logging you may disable logging-fstring-interpolation (I'd recommend that, f-string are very efficient). Also please mind your tone and let's keep this issue focused on consider-using-f-string's false positives.

[DanielNoord]

@Pierre-Sassoulas do you want me to make a PR where consider-using-f-string is no longer raised when logging-fstring-interpolation is enabled?

[Pierre-Sassoulas]

Maybe we need a better documentation about lazy formatting in logging, but there is nothing to change in the code itself as the following correct lazy formatted logging:

import logging

logger = logging.getLogger(__file__)

logger.info('Processes to be spawned: %s', "jobs")

Does not raise any false positives right now.

[sergeyklay]

Oh, you're right. My bad :/ As an excuse, I can only say that I probably wanted to get rid of it too quickly. In any case, a bit more detailed explanation in the documentation would not hurt

[DanielNoord]

@Pierre-Sassoulas I think we can close this issue then, right? Besides the debate on whether this checker should be enabled by default I don't think there are any false positives left in this issue?