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

Q000 fixer does not remove obsolete backslash-escape inside string after changing quotes #8617

Closed
ThiefMaster opened this issue Nov 11, 2023 · 3 comments · Fixed by #8630
Closed
Labels
bug Something isn't working wish Not on the current roadmap; maybe in the future

Comments

@ThiefMaster
Copy link
Contributor

ThiefMaster commented Nov 11, 2023

ruff.toml:

select = ['Q']

[flake8-quotes]
inline-quotes = 'single'
multiline-quotes = 'single'
docstring-quotes = 'double'
avoid-escape = true

Code:

a = "hello\"world"

ruff output:

[adrian@claptrap:/tmp/rufftest]> ~/dev/ruff/target/release/ruff check --no-cache test.py
test.py:1:5: Q000 [*] Double quotes found but single quotes preferred
Found 1 error.
[*] 1 fixable with the `--fix` option.

And after running with --fix added:

a = 'hello\"world'

While not invalid, this is ugly - in particular since avoid-escape is enabled. It should have been this:

a = 'hello"world'
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Nov 11, 2023
@charliermarsh
Copy link
Member

Agreed, it makes sense to remove redundant escapes when converting quote styles.

(I consider this low-priority since we're more focused on string normalization in the formatter, but I would gladly accept a contribution here.)

@charliermarsh charliermarsh added bug Something isn't working wish Not on the current roadmap; maybe in the future and removed rule Implementing or modifying a lint rule labels Nov 11, 2023
@ThiefMaster
Copy link
Contributor Author

I would gladly use the formatter if it had an option to only apply certain changes (such as quote style) since I really do not like what black does to my codebase, so I don't think I'd want to use the ruff formatter either. :/

For implementing a fix, would you do it as part of this fixer of as a new rule (if yes: where, since it's not in the original flake8-quotes) that only looks at unnecessary escapes in a string?

@charliermarsh
Copy link
Member

I hear you! While I don't share that opinion for my own code :) I do understand why saying "Just use the formatter" doesn't close out the issue. I'm more just trying to convey that for our own prioritization, we're more focused on improving the formatter's string normalization than investing further in the linter's string-formatting rules (right now, we basically have to maintain these two separate implementations).

I think easiest would be to add a new rule to the Q set to detect unnecessary escape quotes. We could also change the existing Q rules to take unnecessary escapes into account, but it wouldn't be strictly necessary, since the new rule would fix any unnecessary escapes after the outer quote conversion.

charliermarsh pushed a commit that referenced this issue Nov 13, 2023
When using the autofixer for `Q000` it does not remove the backslashes
from quotes that no longer need escaping.

This new rule checks for such backslashes (regardless whether they come
from the autofixer or not) and can remove them.

fixes #8617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wish Not on the current roadmap; maybe in the future
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants