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

Prevent f-string merge quote changes with nested quotes #4498

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MeGaGiGaGon
Copy link
Contributor

@MeGaGiGaGon MeGaGiGaGon commented Oct 22, 2024

Description

This is my first PR to black, so any guidance is welcome.

This technically fixes, and is strongly related to, #4493, #4494, and #4495

The core theme between then is what I consider a workaround. Currently almost all of the f-string formatting code is commented out and marked todo, which includes the quote normalization. This can be bypassed by using string merging to forcibly change the quote used in f-strings, ie "" f''. From how it looks to me, this is not desirable, as all the same logic and edge cases will be needed whenever that code is ready, which is suboptimal. Ideally only one place would contain all the f-string logic, as from all the encountered edge cases it is quickly becoming very complex.

This is also why none of those mentioned issues are closed by this PR, since their underlying causes/edge cases aren't fixed, just made inaccessible until the f-string code is worked on/some sort of resolution is made.

Since this is a regression, and a very harsh option to fix these, I'd understand if these changes are rejected, in which case this should mostly serve as a question on what to do instead.

Since this is a regression, some tests had to be removed. I was not able to find any documentation on what to do when removing tests, so I just deleted them. They have now been changed so that the output matches with the altered functionality. I also added a simplified version of #4493 as a test case to make sure this fix actually works.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

tests/data/cases/preview_long_strings.py Outdated Show resolved Hide resolved
src/black/trans.py Outdated Show resolved Hide resolved
@MeGaGiGaGon MeGaGiGaGon changed the title Prevent f-string merge quote changes Prevent f-string merge quote changes with nested quotes Nov 5, 2024
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.

2 participants