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 is not enforced with inline-quotes = "single" #11209

Closed
Borda opened this issue Apr 29, 2024 · 9 comments
Closed

Q000 is not enforced with inline-quotes = "single" #11209

Borda opened this issue Apr 29, 2024 · 9 comments
Labels
question Asking for support or clarification

Comments

@Borda
Copy link

Borda commented Apr 29, 2024

following my comment in #7834 (comment) and OpenDevin/OpenDevin#1425 (comment)

We are using v0.4.1 with preco-commit and setting single-quotes but it is not enforced...
This is our config:

[lint]
select = [
    "E", "W", "F",
    "Q",
]

[lint.flake8-quotes]
docstring-quotes = "double"
inline-quotes = "single"

[format]
quote-style = "single"

and this is the sample string which is untouched

logger.info(
    f"Connecting to {username}@{hostname} via ssh. If you encounter any issues, you can try `ssh -v -p {self._ssh_port} {username}@{hostname}` with the password '{self._ssh_password}' and report the issue on GitHub."
)
@charliermarsh
Copy link
Member

I don't think we touch this because it contains single quotes within it, so changing to single quotes would thus require that we escape the inner quotes, and the rule prefers avoiding escapes.

@charliermarsh charliermarsh added the question Asking for support or clarification label Apr 30, 2024
@li-boxuan
Copy link

li-boxuan commented Apr 30, 2024

@charliermarsh The line should actually be this:

f"Connecting to {username}@{hostname} via ssh. "

Full context: https://github.com/xingyaoww/OpenDevin/blob/e38b3ab1efdbadb990988318ecd4a3faf741233a/opendevin/sandbox/docker/ssh_box.py#L178-L182

        logger.info(
            f"Connecting to {username}@{hostname} via ssh. "
            f"If you encounter any issues, you can try `ssh -v -p {self._ssh_port} {username}@{hostname}` with the password '{self._ssh_password}' and report the issue on GitHub. "
            f"If you started OpenDevin with `docker run`, you should try `ssh -v -p {self._ssh_port} {username}@localhost` with the password '{self._ssh_password} on the host machine (where you started the container)."
        )

@li-boxuan
Copy link

https://github.com/zheller/flake8-quotes/, on the other hand, would have caught this line.

@charliermarsh
Copy link
Member

Yeah, we enforce it on the entire implicit concatenation. See: #2400.

@li-boxuan
Copy link

Yeah, we enforce it on the entire implicit concatenation. See: #2400.

Interesting, this "bug fix" seems to be a "bug" to me and the original version seemed more correct 🤣 but I see why you want to choose this.

If one chooses to use single quote, does Q000 attempt to change it to double quotes in this case? From a developer's perspective, I don't care whether it's single quote or double quote - as long as the linter could enforce a consistent behavior, I am happy.

@dhruvmanila
Copy link
Member

If one chooses to use single quote, does Q000 attempt to change it to double quotes in this case?

No, the rule wouldn't convert a single-quoted string to a double-quoted string if the user preferred the "single" quotes in their config. But, in your case, there's a single quote in the string content which is why Ruff doesn't flag the string. This is to maintain consistency across all the parts of an implicitly concatenated string. For example, the following is inconsistent because some parts uses single quotes while others uses double quotes.

        logger.info(
            f'Connecting to {username}@{hostname} via ssh. '
            f"If you encounter any issues, you can try `ssh -v -p {self._ssh_port} {username}@{hostname}` with the password '{self._ssh_password}' and report the issue on GitHub. "
            f"If you started OpenDevin with `docker run`, you should try `ssh -v -p {self._ssh_port} {username}@localhost` with the password '{self._ssh_password} on the host machine (where you started the container)."
        )

I think in this case you might get away with using !r instead of explicit quotes:

        logger.info(
            f'Connecting to {username}@{hostname} via ssh. '
            f'If you encounter any issues, you can try `ssh -v -p {self._ssh_port} {username}@{hostname}` with the password {self._ssh_password!r} and report the issue on GitHub. '
            f'If you started OpenDevin with `docker run`, you should try `ssh -v -p {self._ssh_port} {username}@localhost` with the password {self._ssh_password!r} on the host machine (where you started the container).'
        )

This should put the quotes around the self._ssh_password value. For example:

In [3]: print(f'hello {x!r}')
hello 'world'

Does this help?

@charliermarsh
Copy link
Member

The behaviors here are intentional and I think we're unlikely to make a bunch of changes to the rule, since we generally recommend using the formatter to enforce this anyway (which also allows specifying single-quote for inline with double-quote for docstrings).

@li-boxuan
Copy link

Thanks, I think we can live with that protocol. @Borda what do you think? Can we close the issue now?

@charliermarsh
Copy link
Member

Closing for now as "working as intended", hopefully clarified by the above!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

No branches or pull requests

4 participants