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

Do not consider f-strings with escaped newlines as multiline #14624

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

dhruvmanila
Copy link
Member

Summary

This PR fixes a bug in the f-string formatting to not consider the escaped newlines for is_multiline. This is done by checking if the f-string is triple-quoted or not similar to normal string literals.

This is not required to be gated behind preview because the logic change for is_multiline was added in #14454.

Test Plan

Add a test case which formats differently on main: https://play.ruff.rs/ea3c55c2-f0fe-474e-b6b8-e3365e0ede5e

@dhruvmanila dhruvmanila added bug Something isn't working formatter Related to the formatter labels Nov 27, 2024
Copy link
Contributor

github-actions bot commented Nov 27, 2024

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser added the preview Related to preview mode features label Nov 27, 2024
@MichaReiser
Copy link
Member

This is not required to be gated behind preview because the logic change for is_multiline was added in #14454.

I'm not sure I understand this part. Isn't the reason why no preview gating is required because this new logic is only used in preview mode?

@dhruvmanila
Copy link
Member Author

Isn't the reason why no preview gating is required because this new logic is only used in preview mode?

The is_multiline is also used in binary expression formatting which isn't gated behind preview. You can see that in the playground by turning off the preview flag: https://play.ruff.rs/8ef99e9b-ea54-44b4-a483-f7339424865d. The logic for f-strings was changed from just checking for newlines in the entire f-string to checking individual elements in the linked PR.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at #14454 is_multiline again, I think the change we made there has a high potential for unwanted stable style changes. We should change the is_multiline to keep using the "old" memchr2(b'\n', b'\r', source[fstring.range].as_bytes()).is_some() if f-string formatting is disabled and only use the new logic if f-string formatting is enabled. We otherwise risk that bugs like the one you found here impact stable style users.


# This is not a multiline f-string, but the expression is too long so it should be
# wrapped in parentheses.
f"hellooooooooooooooooooooooo \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add some tests in assignment positions and with implicit concatenated strings (especially with inline comments) just to make sure strings with line continuations are handled correctly in those positions too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these test case reveals one more change in implicitly concatenated strings. Given:

f"hellooooooooooooooooooooooo \
        worldddddddddddddddddd" "another string"

On main, we don't concatenate it because the f-string is considered as multiline but here we would. I'm going to make the is_multiline change gated behind preview first, rebase this PR and add the test cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think concatenating the string makes sense as long as it fits into the line length

@dhruvmanila
Copy link
Member Author

I think the change we made there has a high potential for unwanted stable style changes. We should change the is_multiline to keep using the "old" memchr2(b'\n', b'\r', source[fstring.range].as_bytes()).is_some() if f-string formatting is disabled and only use the new logic if f-string formatting is enabled. We otherwise risk that bugs like the one you here impact users of our stable style.

I see, I think that makes sense. I'll do that as a follow-up.

@dhruvmanila dhruvmanila force-pushed the dhruv/f-string-multiline-triple-quoted branch from 3b8a1cc to 2573374 Compare November 27, 2024 09:45
@dhruvmanila dhruvmanila changed the base branch from main to dhruv/f-string-multiline-preview November 27, 2024 09:46
dhruvmanila added a commit that referenced this pull request Nov 27, 2024
## Summary

Ref:
#14624 (review)

## Test Plan

The test case in the follow-up PR showcases the difference between
preview and non-preview formatting:
https://github.com/astral-sh/ruff/pull/14624/files#diff-dc25bd4df280d9a9180598075b5bc2d0bac30af956767b373561029309c8f024
Base automatically changed from dhruv/f-string-multiline-preview to main November 27, 2024 10:20
@dhruvmanila dhruvmanila force-pushed the dhruv/f-string-multiline-triple-quoted branch from 2573374 to d720c51 Compare November 27, 2024 10:20
@dhruvmanila dhruvmanila enabled auto-merge (squash) November 27, 2024 10:22
@dhruvmanila dhruvmanila merged commit f96fa6b into main Nov 27, 2024
20 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/f-string-multiline-triple-quoted branch November 27, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter Related to the formatter preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants