Replace percent, string concat, format calls with f-strings #5890
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the codebase by replacing all str.format() calls, % operator, and most string concatenation with f-string literals, following modern Python best practices.
- Replaces percentage string formatting (
%) with f-strings - Replaces
str.format()calls with f-strings - Updates string concatenation patterns to use f-strings where appropriate
Reviewed Changes
Copilot reviewed 87 out of 87 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_ui.py | Replaces percentage formatting and string concatenation with f-strings |
| test/test_logging.py | Converts string concatenation in logging calls to f-strings |
| test/test_library.py | Updates percentage formatting to f-strings |
| test/test_dbcore.py | Converts SQL query formatting to f-strings |
| test/test_datequery.py | Replaces string concatenation with f-strings |
| test/test_art_resize.py | Simplifies command construction with list unpacking |
| test/plugins/ | Multiple plugin test files updated to use f-strings |
| docs/ | Documentation examples updated to use f-strings |
| beetsplug/ | Plugin modules updated to use f-strings throughout |
| beets/ | Core module files updated with f-string formatting |
snejus
force-pushed
the
use-f-string
branch
2 times, most recently
from
90d5f7e to
a05f9fc
Compare
|
I just took a quick look, and I strongly disagree with using f-strings for SQL transactions. Interpolating values directly into SQL queries using f-strings (or .format) exposes users to SQL injection vulnerabilities, which is a major security risk. Instead, we should always use parameterized queries, like: tx.query("INSERT INTO foo VALUES (%s, %s, %s, %s, %s)", (num, string, ...))
# or even
tx.query("INSERT INTO foo VALUES (?,?,?)", data)fyi: this is also mentioned in the sqlite3 docs. It might also be worth looking into backport templatestrings as they would fix this issue. https://docs.python.org/3.14/library/string.templatelib.html Addition: It seems like we are good on that front and only use fstring for table names 👍 We should still add a note to the contribution guide, as at the moment it is a bit misleading. |
My intention here was to replace Did I mistakenly replace SQLite args with an f-string somewhere? |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
I don't think you did, looks well done to me 👍 I was a bit worried because I read the docs changes first which are now a bit misleading. Would it be possible to add a single sentence to the dev docs? Something alike to:
|
Is it necessary? Plugin developers are expected to use our query wrappers instead of tinkering with SQL directly. I was looking at |
|
I was thinking in the contribution guide, where we show the g.lib.transaction examples as this was why I was a bit scared/confused initially. I guess at the end of the Underneath it, we say use f-string, which might be misleading imo. But you are right, on the other hand we might be fine with just hoping everyone who goes this deep into our codebase knows what they are doing. We always have reviews to catch such issue. |
|
@semohr I added a couple of more commits to standardise our logging calls. |
Lady Madonna apparently is gone from this website. ¯\_(ツ)_/¯
This PR modernizes the codebase by replacing all
str.format()calls,%operator, and most of string concatenation with f-string literals.Fixes #5293
Supersedes #5337
Once this is reviewed, I will squash all commits into one and add the hash to
.git-blame-ignore-revsfile.