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

Implement B028 rule from bugbear #1893

Closed
LefterisJP opened this issue Jan 15, 2023 · 4 comments
Closed

Implement B028 rule from bugbear #1893

LefterisJP opened this issue Jan 15, 2023 · 4 comments
Labels
rule Implementing or modifying a lint rule

Comments

@LefterisJP
Copy link

flake8-bugbear just released an update to add an interesting rule: https://pypi.org/project/flake8-bugbear/

B028: Consider replacing f"'{foo}'" with f"{foo!r}" which is both easier to read and will escape quotes inside foo if that would appear. The check tries to filter out any format specs that are invalid together with !r. If you’re using other conversion flags then e.g. f"'{foo!a}'" can be replaced with f"{ascii(foo)!r}". Not currently implemented for python<3.8 or str.format() calls.

It hits in some places in rotki's codebase but I am not gonna go change them all manually 😄

It would be great if ruff also detects it and auto-fixes it.

@LefterisJP
Copy link
Author

Hmm okay now that I am looking at it with a closer look though I think this rule may not be as smart as its definition makes it out to be. Take the following code for example:

class Person:

    def __init__(self, name: str, age: int) -> None:
        self.name = name
        self.age = age

    def __str__(self) -> str:
        return f'{self.name} who is {self.age} years old'

    def __repr__(self) -> str:
        return f'{id(self)}'


john = Person('John', 24)
print(f'"{john}"')
print(f'{john!r}')

Since __repr__ is implemented, the !r version is different.

"John who is 24 years old"
140039984729776

Running flake8 with that bugbear rule active still seems to advice to change it:

test.py:17:7: B028 'john' is manually surrounded by quotes, consider using the `!r` conversion flag.

And doing so would be a bug.

I wonder if it would be possible to only make it warn you if str == repr for the variable in question.

@peterjc
Copy link

peterjc commented Jan 16, 2023

@LefterisJP interesting test case, even numeric types have a problem - see PyCQA/flake8-bugbear#329 and also PyCQA/flake8-bugbear#332 - it looks likely the B028 check will be disabled by default

@LefterisJP
Copy link
Author

@LefterisJP interesting test case, even numeric types have a problem - see PyCQA/flake8-bugbear#329 and also PyCQA/flake8-bugbear#332 - it looks likely the B028 check will be disabled by default

Yeah I think this rule may not be a good idea at all. I will leave it to @charliermarsh to decide but feel free to close.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jan 16, 2023
@charliermarsh
Copy link
Member

Ah yeah, ok, looks like it's being renamed to B907, so lets close for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

3 participants