Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consider-using-f-string produces worse quality code in some cases #5144

Closed
doublethefish opened this issue Oct 11, 2021 · 2 comments
Closed
Labels
Discussion 🤔 Enhancement ✨ Improvement to a component

Comments

@doublethefish
Copy link
Contributor

doublethefish commented Oct 11, 2021

Bug description

consider-using-f-string produces worse quality code in some cases, for example consider the following format string:

            "Timing of '{name}' took {secs}s/{total_secs}s (started {start_time})".format(
                name=name,
                secs=delta.total_seconds(),
                total_secs=self._config.timeout_seconds,
                start_time=self._start_time.strftime("%H:%M:%S %Z").strip(),
            )

Any variant of using an f-string to remove the consider-using-f-string warning does one of the following:

  • causes more local variables to be created (possibly blowing the max-locals limit)
  • is less readable (as the context of what the pieces are intended for is lost, e.g. took {secs}s/{total_secs}s is significantly more readable than took {delta.total_seconds()}s/{self._config.timeout_seconds}s
  • splits the context over multiple lines, making it harder to read the context

Configuration

No response

Command used

python3 -m pylint -j4 --score=n --rcfile=.pylintrc <files>

Pylint output

[..snip..]
C0209: Formatting a regular string which could be a f-string (consider-using-f-string)
[..snip..]

Expected behavior

Only emit the warning for invocations of str.format that are below a certain complexity i.e. 5 parameters.

Additionally only emit the warning when the conformed string is

  1. within line-width limits (including indents), or
  2. roughly the same length as the string it would replace - say maximally 10% larger than the existing string (as f-strings are smaller in most cases).

Pylint version

pylint 2.11.1
astroid 2.8.2
Python 3.10.0 (default, Oct  6 2021, 00:30:31) [GCC 10.3.1 20210424]

OS / Environment

n/a but
Alpine Linux 9b0fb0d01a22 5.10.47-linuxkit #1 SMP PREEMPT Sat Jul 3 21:50:16 UTC 2021 aarch64 Linux
bash

Additional dependencies

No response

@doublethefish doublethefish added Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Oct 11, 2021
@Pierre-Sassoulas Pierre-Sassoulas added Discussion 🤔 Enhancement ✨ Improvement to a component and removed Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Oct 11, 2021
@doublethefish
Copy link
Contributor Author

Closing as most of this has already been considered in #5039, although not all points are directly considered.

@Pierre-Sassoulas
Copy link
Member

Thank you for searching for other issue related to that @doublethefish 😄. For reference if someone happen to see this issue following a search, in the example given it's possible to do:

            template = "Timing of '{name}' took {secs}s/{total_secs}s (started {start_time})"
            template.format(
                name=name,
                secs=delta.total_seconds(),
                total_secs=self._config.timeout_seconds,
                start_time=self._start_time.strftime("%H:%M:%S %Z").strip(),
            )

And not have any warnings raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion 🤔 Enhancement ✨ Improvement to a component
Projects
None yet
Development

No branches or pull requests

2 participants