astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.41k stars 1.08k forks source link

Use self-documenting f-strings where possible #4302

Open janosh opened 1 year ago

janosh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 🤔

janosh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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