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

Fix crash with f-string docstrings #4019

Merged
merged 3 commits into from Nov 7, 2023
Merged

Conversation

hauntsaninja
Copy link
Collaborator

Fixes #4018

Python does not consider f-strings to be docstrings, so we probably shouldn't be formatting them as such. This is the easiest way to fix the issue with the AST check.

Despite its name, I think docstring_preview.py might be testing stable style, which this unfortunately does change

Python does not consider f-strings to be docstrings, so we probably
shouldn't be formatting them as such

Fixes psf#4018

Despite its name, I think docstring_preview.py might be testing stable
style, which this unfortunately does change
src/black/nodes.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link

github-actions bot commented Nov 3, 2023

diff-shades reports zero changes comparing this PR (c548676) to main (c54c213).


What is this? | Workflow run | diff-shades documentation

@JelleZijlstra
Copy link
Collaborator

Thanks, it does look like this changes the stable style. The case it changes is rather odd, but we should still follow our policy and make the change only in the stable style.

Some options:

  • Just wait two months and merge this PR in January so it can go straight into the stable style, without waiting for preview. Seems OK since the change is rather obscure.
  • For now, keep formatting f-strings without any placeholders, and turn off formatting for f-strings with placeholders. This would fix the crash while preserving behavior. In the preview style we can also disable formatting for f-strings without placeholders.
  • Fix the crash instead by loosening the AST safety check (which already special cases docstrings). I'd rather not do that though.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Nov 3, 2023

Option 2 would also change the stable style right?

I would also like to avoid Option 3. Note afaict that the AST safety check special cases all strings, not just docstrings, which makes me even more loathe to take Option 3.

So Option 1 it is?

@JelleZijlstra
Copy link
Collaborator

I thought Option 2 was safe because any change we make to f-strings with placeholders would cause an AST safety crash. Though I guess it would change the stable style under --fast? I would say that's OK but we haven't specified it explicitly.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Nov 3, 2023

Oh, it's not any f-string with placeholders. It's f-strings with leading or trailing space with placeholders adjacent to that space. I guess we could detect that specific situation and not format, but I'd prefer to have the behaviour be consistent and predictable, especially since Python does not consider f-strings to be docstrings.

@hauntsaninja
Copy link
Collaborator Author

Actually wait, there is no stability concern. The stability guarantee is just that code that was previously formatted with Black will not change, not that formatting unformatted code will lead to the same results.

@JelleZijlstra JelleZijlstra merged commit ecbd9e8 into psf:main Nov 7, 2023
41 checks passed
@hauntsaninja hauntsaninja deleted the fstrdoc branch November 7, 2023 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal error on a specific file
3 participants