astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.34k stars 1.08k forks source link

Rule Request: redundant logfmt str ctor catch in G rules or add new rule #12734

Open Skylion007 opened 2 months ago

Skylion007 commented 2 months ago

This is a really common antipattern I wish logfmt (G*) rules would catch:

   log.warning("Cannot write to %s", str(profile_path))

is redundant and does an potentially expensive str construction which is what we are trying to avoid with these rules anyway. Instead, the %s ensure it will be formatted as a string anyway! As such, this can be simplified into the better code:

   log.warning("Cannot write to %s", profile_path)

Which will be faster in both the case where it's printed and the case where it's not by either avoiding an unnecessary string copy or a unnecessary construction in the first place.

This rule could be added more generically to any string formatting in the new or old styles

adamchainz commented 2 months ago

Replying on behalf of flake8-logging here. I think we can avoid parsing the string formatting the message and instead error for any str or repr calls in the positional arguments of a logging call. That seems like it would catch the issue without much chance of false positive. What do you think? Happy to review a PR there.

charliermarsh commented 2 months ago

Thanks! We do have a parser for format strings so we could parse it and cross-reference with the positional calls if needed.