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

convert f-string with nothing to interpolate to regular string #1156

Open
raybuhr opened this issue Nov 12, 2019 · 9 comments
Open

convert f-string with nothing to interpolate to regular string #1156

raybuhr opened this issue Nov 12, 2019 · 9 comments
Labels
F: strings Related to our handling of strings T: enhancement New feature or request

Comments

@raybuhr
Copy link

raybuhr commented Nov 12, 2019

Is your feature request related to a problem? Please describe.

f-strings are great. sometimes they are so great we might use them even when we don't need to, such as in files where an f-string is used but nothing to interpolate was included in the string body.

Describe the solution you'd like

say you had a python script with a line like this:

hello = f"hello"

black should see that f-string, recognize that it could just be a regular string and drop the f-prefix.

output:

hello = "hello"

Describe alternatives you've considered

Could return an error message, but I want black to format for me.
Could also not do this, but I think changing to a regular string is a better option.

@raybuhr raybuhr added the T: enhancement New feature or request label Nov 12, 2019
@raybuhr raybuhr changed the title convert f-string without nothing to interpolate to regular string convert f-string with nothing to interpolate to regular string Nov 12, 2019
@vemel
Copy link
Contributor

vemel commented Nov 19, 2019

This can potentially be dangerous for f-strings with brackets.

s = f"this can be safely transformed to a regular string"
s2 = f"Here we have {{double_brackets}}, should they be replaced with single? Who knows..."

@vemel
Copy link
Contributor

vemel commented Nov 19, 2019

As I see from the source code, f-string parsing is very poor for now.

Suggestion: what if we use ast.NodeVisitor for regular strings and f-strings and try to build a result from a tree.

  • no regexps needed to parse f-strings or choose quotes
  • quotes can be chosen from a list: ", ', """", ''''. if a new string uses qute from the list, we just pick the next available option. if we are out of quote options - abort formatting, Leaf
    stays as it is.
  • no need to implement full AST-to-string, visit_str, visit_bytes, visit_JoinedStr, visit_Num, visit_Name, visit_NameConstant should be enough. On unknown node - abort formatting, Leaf
    stays as it is.

I have some drafts that could help.

@bbugyi200
Copy link
Contributor

bbugyi200 commented Mar 15, 2020

#1132 sets up the ground work for this. Here is an example of how long f-strings should be expected to behave after this pull request is merged:

##### INPUT
fstring = f"f-strings definitely make things more {difficult} than they need to be for {{black}}. But boy they sure are handy. The problem is that some lines will need to have the 'f' whereas others do not. This {line}, for example, needs one."

##### OUTPUT
fstring = (
    f"f-strings definitely make things more {difficult} than they need to be for"
    " {black}. But boy they sure are handy. The problem is that some lines will need"
    f" to have the 'f' whereas others do not. This {line}, for example, needs one."
)

As you can see, the f prefix is dropped for the middle string because it was not needed.

I had thought about implementing this in a more general form (as this issue requests). Converting f"hello" to "hello" modifies the AST, however, so I chose against it.

@reedstrm
Copy link

flake8 version 3.8.3 had error "F541 f-string is missing placeholders" that trips on exactly those kind of strings. While I understand the bright-line of "black doesn't change the AST", I have sort of gotten used to the crutch that "black will fix all the flake8 formatting problems" :) From an coder-in-the-editor perspective, removing the "f" from the middle string above feels about the same as removing it from a bare f"Hello".

@JelleZijlstra
Copy link
Collaborator

I'm OK with doing this; we already make known to be harmless AST changes in a few places. I believe on master we already remove useless fs in a few contexts.

@JelleZijlstra JelleZijlstra added the F: strings Related to our handling of strings label May 30, 2021
@zsol
Copy link
Collaborator

zsol commented May 28, 2022

Wouldn't a linter be a better place for this type of transformation? Flake8 already flags this, but fails to offer an autofix - which IMO is why it's even considered for Black. If flake8 automatically fixed this for you, Black wouldn't need to.

@JelleZijlstra
Copy link
Collaborator

I feel like in general it's Black's job to make simple, safe formatting fixes.

@dougthor42
Copy link
Contributor

Half the time I forget to add the f prefix. The other half the time I forget to add the curly brackets around my var. I think automatically dropping a "looks useless to me" f prefix is a bad idea. How can black know what the user intended?

print(f"myvar=")
print("{myvar=}")

The same argument applies to the "automatically add f prefix.

Sorry, but 👎 for me.

@Nodd
Copy link

Nodd commented Jun 2, 2023

Ruff can fix it automatically : https://beta.ruff.rs/docs/rules/f-string-missing-placeholders/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: strings Related to our handling of strings T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants