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 for multiline strings with different quotes #2400

Closed
samuelcolvin opened this issue Jan 31, 2023 · 13 comments · Fixed by #2416
Closed

Q000 for multiline strings with different quotes #2400

samuelcolvin opened this issue Jan 31, 2023 · 13 comments · Fixed by #2416
Assignees
Labels
bug Something isn't working

Comments

@samuelcolvin
Copy link

code:

    assert s.to_python(123) == (
        "123 info=SerializationInfo(include=None, exclude=None, mode='python', by_alias=True, exclude_unset=False, "
        "exclude_defaults=False, exclude_none=False, round_trip=False)"
    )

Is resulting in "Q000 Double quotes found but single quotes preferred"

As you can see the first line has a single quote in it, so needs to use double quotes, the second line doesn't but by conversion you'd expect to keep using double quotes for the whole multi-line string.

@samuelcolvin
Copy link
Author

Tried on latest ruff, getting the same error.

@charliermarsh charliermarsh added the bug Something isn't working label Jan 31, 2023
@charliermarsh
Copy link
Member

Makes sense! Thanks for filing.

@samuelcolvin
Copy link
Author

samuelcolvin commented Jan 31, 2023

Just for reference, same error reported on flake8-quotes by me in 2019 - zheller/flake8-quotes#82

@charliermarsh
Copy link
Member

Hopefully I can fix it today :)

@charliermarsh
Copy link
Member

Do you think this should be an error, or merely permitted?

    assert s.to_python(123) == (
        "123 info=SerializationInfo(include=None, exclude=None, mode='python', by_alias=True, exclude_unset=False, "
        'exclude_defaults=False, exclude_none=False, round_trip=False)'
    )

@samuelcolvin
Copy link
Author

personally I would say an error, but I'm fine with permitted too, especially since you had to do that (or add noqa) until now.

@charliermarsh
Copy link
Member

I did some testing -- I probably need to treat it as "permitted", because Black changes them back. That is, Black changes this:

(
    '"abc"'
    'abc'
)

...to...

(
    '"abc"'
    "abc"
)

(Black will also truncate the whitespace, but omitted that change for clarity.)

It wouldn't be a huge deal in practice, since if you're using this plugin with "double" preference, you're compatible with Black anyway; and if you're using it with "single" preference, you're already incompatible with Black. But I'd prefer to avoid the potential of introducing a fix loop.

@samuelcolvin
Copy link
Author

agreed

@charliermarsh charliermarsh self-assigned this Jan 31, 2023
charliermarsh added a commit that referenced this issue Jan 31, 2023
…erred quote (#2416)

As an example, if you have `single` as your preferred style, we'll now allow this:

```py
assert s.to_python(123) == (
    "123 info=SerializationInfo(include=None, exclude=None, mode='python', by_alias=True, exclude_unset=False, "
    "exclude_defaults=False, exclude_none=False, round_trip=False)"
)
```

Previously, the second line of the implicit string concatenation would be flagged as invalid, despite the _first_ line requiring double quotes. (Note that we'll accept either single or double quotes for that second line.)

Mechanically, this required that we process sequences of `Tok::String` rather than a single `Tok::String` at a time. Prior to iterating over the strings in the sequence, we check if any of them require the non-preferred quote style; if so, we let _any_ of them use it.

Closes #2400.
@samuelcolvin
Copy link
Author

Wonderful, thanks.

@charliermarsh
Copy link
Member

Is there any branch etc. that I could test this on just to ensure compatibility? :)

It'll go out in tonight's release.

@samuelcolvin
Copy link
Author

See pydantic/pydantic-core#374, there's currently a bunch of noqas.

@charliermarsh
Copy link
Member

👍 Thanks, looking good on that branch

@samuelcolvin
Copy link
Author

👍 (on mobile, so this is a reaction)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants