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

start UP031 by fixing simple %s strings #10266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

start UP031 by fixing simple %s strings #10266

wants to merge 1 commit into from

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Jan 3, 2025

Part of #10196

Before embarking on a journey to fix (or ask people to open many PRs) all of the issues I decided to open a PR with some of the simple fixes.

These were all suggested as "unsafe" fixes by ruff. They're unsafe because if the value was a tuple instead of a string then the f string behaves slightly differently. However, I've examined each of these examples pretty closely and don't see any cases where a tuple is likely to slip in.

Technical

https://docs.astral.sh/ruff/rules/printf-string-formatting/

Testing

I've clicked around on much of the site locally and didn't hit any errors but I didn't extensively check that all the code changed is triggered.

Screenshot

Stakeholders

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Jan 4, 2025
@mekarpeles
Copy link
Member

Just wanted to confirm that we used --fix (with no unsafe substitutions)

If so, happy to merge, thank you!

@mekarpeles mekarpeles self-assigned this Jan 6, 2025
@mekarpeles mekarpeles added the Priority: 3 Issues that we can consider at our leisure. [managed] label Jan 6, 2025
@RayBB
Copy link
Collaborator Author

RayBB commented Jan 7, 2025

@mekarpeles these are small subset of the changes made with the --unsafe-fixes flag that I have checked closely.
When you use that flag they offer many different fixes using .format and the like but I just picked out a set of the simplest ones.
As mentioned in the docs they only reason it's "unsafe" is because they can't know if the value is a tuple or not.
I've examined each of these cases to check if a tuple could get passed in and I don't see any case where anything but a string/int would be coming in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects: Developers Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. Priority: 3 Issues that we can consider at our leisure. [managed]
Projects
Status: Waiting Review/Merge from Staff
Development

Successfully merging this pull request may close these issues.

2 participants