search

beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.57k stars 1.8k forks source link

Replace most `str.format()` uses with f-strings #5337

Open bal-e opened 4 days ago

bal-e commented 4 days ago

Fixes #5293.

Along the way, I've made some small code improvements beyond just the use of f-strings; I don't think these will conflict with others' work. In particular, I've avoided modifying the formatting of paths; there is work to greatly simplify that using 'pathlib', at which point a second commit can clean them up.

github-actions[bot] commented 4 days ago

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.

RollingStar commented 4 days ago

Thank you for the monster commit. Hopefully you had automation to aid you. I only looked at the first few chunks (about as far as my final comment) but it passes the smell test.

I would prefer having the SQL changes in their own commit. Anything with SQL should probably have more attention on it than an f-string.

bal-e commented 4 days ago

Thank you for the monster commit. Hopefully you had automation to aid you. I only looked at the first few chunks (about as far as my final comment) but it passes the smell test.

Thanks for reviewing! This is a pretty big change and I am worried I slipped up somewhere, so I'm happy to hang on to the PR until somebody can look at the full diff. PyCharm helped me find the usages of str.format() but I modified them by hand.

I would prefer having the SQL changes in their own commit. Anything with SQL should probably have more attention on it than an f-string.

That's understandable, messing with SQL can be dangerous. The tests pass, which I hope adds some degree of confidence to these changes. I haven't fundamentally changed any SQL statements, though -- they should operate exactly the same as before. I can refactor them out to a separate commit, but I think that should wait until somebody reviews the entire diff.

Serene-Arc commented 4 days ago

I'll review the entire diff and get back to you.