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

query: UP032 fix leaves bad formatting? #8683

Closed
lucascolley opened this issue Nov 14, 2023 · 6 comments · Fixed by #8712
Closed

query: UP032 fix leaves bad formatting? #8683

lucascolley opened this issue Nov 14, 2023 · 6 comments · Fixed by #8712
Assignees
Labels
fixes Related to suggested fixes for violations

Comments

@lucascolley
Copy link

noted by @ilayn scipy/scipy#19516 (comment)

version: ruff 0.1.5

UP032 fix seems to leave some undesired formatting. In the case of SciPy where we want to run the linter without a formatter, this is less than ideal. Is this the intended behaviour, or could it be improved? Examples:

- raise ValueError("Conflicting configuration dicts: {!r} {!r}"
-                  "".format(new_dict, d))
+ raise ValueError(f"Conflicting configuration dicts: {new_dict!r} {d!r}"
+                  "")
+ raise ValueError("invalid number of data points ({}) specified"
+                  .format(tmp.shape[axis]))
- raise ValueError(f"invalid number of data points ({tmp.shape[axis]}) specified"
-                  )
@charliermarsh
Copy link
Member

Fix formatting is very much a best-effort thing, but we can definitely try to improve these. (I might argue that the first one had strange formatting before the fix too :))

@charliermarsh charliermarsh added the fixes Related to suggested fixes for violations label Nov 14, 2023
@lucascolley
Copy link
Author

I might argue that the first one had strange formatting before the fix too

Definitely 😆 I don't know how much code is formatted like this in the wider world but there are quite a few cases of this in scipy/scipy#19516

@ilayn
Copy link

ilayn commented Nov 14, 2023

first one had strange formatting before the fix too

Indeed and I am responsible for a few of those. But in our defense, the reasoning is the PEP8 80 chars line limit olympics.

We can live with the 2nd example though not really a big deal.

@charliermarsh
Copy link
Member

It'd be nice to detect-and-strip trailing empty strings and trailing newlines.

@charliermarsh charliermarsh self-assigned this Nov 16, 2023
charliermarsh added a commit that referenced this issue Nov 16, 2023
## Summary

When converting from a `.format` call to an f-string, we can trim any
trailing empty tokens.

Closes #8683.
@charliermarsh
Copy link
Member

Hopefully better in the next release! At least, the two examples above are fixed by #8712.

@ilayn
Copy link

ilayn commented Nov 16, 2023

@charliermarsh thank you for the rapid action. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants