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

Use self-documenting f-strings where possible #4302

Open
janosh opened this issue May 9, 2023 · 7 comments
Open

Use self-documenting f-strings where possible #4302

janosh opened this issue May 9, 2023 · 7 comments
Labels
rule Implementing or modifying a lint rule

Comments

@janosh
Copy link

janosh commented May 9, 2023

Could maybe be added to flake8-simplify.

my_var = 42_000

# bad (if target=py3.8+)
f"my_var={my_var}"
f"my_var = {my_var}"
f"my_var = {my_var:,}"

# good
f"{my_var=}"
f"{my_var = }"
f"{my_var = :,}"
@ngnpope
Copy link
Contributor

ngnpope commented May 9, 2023

This would be nice, and would help make those who don't know this feature aware of it 🙂

The only snag is that f-strings are not part of the formal grammar until Python 3.12 - see PEP 701 - so they're tricky to do much with right now.

Other test cases to include for unbalanced spacing:

# bad (if target=py3.8+)
f"my_var= {my_var}"
f"my_var ={my_var}"

# good
f"{my_var= }"
f"{my_var =}"

@akx
Copy link
Contributor

akx commented May 9, 2023

I think this is a RUF rule or FLY (recently introduced in #4196, see https://github.com/ikamensh/flynt) rule, but sounds like a good idea :)

@akx
Copy link
Contributor

akx commented May 9, 2023

Ah, this is made difficult by the fact that ruffpython-ast apparently already parses f"{my_var=}" as

[
  Located { range: 119..131, custom: (), node: Constant { value: Str("my_var="), kind: None } },
  Located { range: 119..131, custom: (), node: FormattedValue {
    value: Located { range: 122..128, custom: (), node: Name { id: "my_var", ctx: Load } },
    conversion: 114,
    format_spec: None
  }}
]

so it's not easy to distinguish between already correctly formed expressions and those that need changing 🤔

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label May 9, 2023
@janosh
Copy link
Author

janosh commented May 9, 2023

Tbh I'm out of my depth on AST and grammar. At the risk of going against orthodoxy and sounding low-status, if the grammar doesn't formalize f-strings below 3.12, maybe a regex can help distinguish true positives? 😄

@MichaReiser
Copy link
Member

Ah, this is made difficult by the fact that ruffpython-ast apparently already parses f"{my_var=}" as

[
  Located { range: 119..131, custom: (), node: Constant { value: Str("my_var="), kind: None } },
  Located { range: 119..131, custom: (), node: FormattedValue {
    value: Located { range: 122..128, custom: (), node: Name { id: "my_var", ctx: Load } },
    conversion: 114,
    format_spec: None
  }}
]

so it's not easy to distinguish between already correctly formed expressions and those that need changing thinking

The two nodes differ in that the second has conversion set to b'r'. I don't know why conversion is an usize instead the ConversionFlags that RustPython uses internally or just the char (@youknowone do you know why?). But I think it could allow us to tell the two formats apart.

https://github.com/charliermarsh/RustPython/blob/50b8570bc262098b84dac86092133d3ee26caf23/compiler/core/src/bytecode.rs#L328-L342

@youknowone
Copy link
Contributor

youknowone commented May 10, 2023

Python.asdl refers it as int and RustPython naively convert it to actual int. We didn't care about it that much because we were the sole ast user, but that need to be enhanced now.
There are 7 int values and 2 refers bool and another one is conversion.
I recently changes bool values to actual bool and going to work on conversion too.
It will be ConversionFlag or Option<ConversionFlag>. I didn't investigate enough which one is more correct.

@youknowone
Copy link
Contributor

About this or not, please feel free to suggest anything I can make ast better.

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

6 participants