-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add PEP701 support #3822
Add PEP701 support #3822
Conversation
@ambv This is the approach that I'm thinking of: defining a Does the approach seem ok? If yes, I'll go ahead and implement all the missing features. |
cc @JelleZijlstra as well, for the above comment. |
Also, although I don't think this change will be backwards incompatible, should it be possible to keep the old behaviour intact? Should this be behind the experimental flag? |
What aspect won't be backward compatible? Shouldn't we just start parsing code we previously failed on? |
@JelleZijlstra I thought so too. But I'm not 100% confident. Do we want to preserve parsing failures for 3.11 and before? Or can we just drop the old tokenizer and parser? |
I think the proposed approach is wrong. You can't use the existing understanding of strings and separately decompose them. You don't know the endquote up front anymore because this is now valid: f = f"abc{"def"}" You need to use a stack inside the existing |
@tusharsadhwani It's fine if we parse this code in earlier Python versions too. Catching syntax errors is not a goal for Black. |
@ambv wouldn't the stack frames made by calling You can think of every call to |
No, the call stack isn't automatically compatible with the stack we need here. You need to change how When you encounter That way you can encounter another At no point are you iterating a counter like Is that clearer now? |
Yes, it's clearer now. I'll implement the rest. |
$ python -m blib2to3.pgen2.tokenize
f'foo \{2+2}'
1,0-1,2: FSTRING_START "f'"
1,2-1,12: FSTRING_MIDDLE 'foo \\{2+2}'
1,12-1,13: FSTRING_END "'"
1,13-1,14: NEWLINE '\n' |
@JelleZijlstra everything in |
Thank you! Testing on our internal codebase found another crash:
produces
Interestingly this happens only if there is a newline in the inner f-string. This is Python 3.9 code, so valid before and after PEP 701. |
Other than the crashes though, this looks good and I'm hopeful we can merge it soon. We should add a preview style feature to enable formatting inside f-strings too, but that can wait for another PR. |
Cool, let me take a look. |
@JelleZijlstra edge case taken care of. |
Thanks! I ran the branch on our company repo and on a venv full of interesting installed packages and found no more bugs. I'll read over the diff one more time and hopefully then we'll be ready. |
tests/data/cases/pep_701.py
Outdated
f"{ 2 + 2 = }" | ||
|
||
# TODO: | ||
# f"""foo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it easy to fix this TODO in Black? I checked and this syntax works in 3.11 and with current Black, so it would be a regression if we start failing on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed now.
f'{(abc:=10)}' | ||
|
||
f"This is a really long string, but just make sure that you reflow fstrings { | ||
2+2:d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a SyntaxError in 3.12.0 but not 3.12.1, I suppose due to python/cpython#112059. Ran into this because I ran the test suite on 3.12.0 and it failed.
No change requested, but it seems possible this will cause trouble again in the future.
WITH {f''' | ||
{1}_cte AS ()'''} | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding this test case:
f"{
X
!r
}"
It works already, but good to cover it in the test suite.
#4321 somehow broke changes in this PR, even though visit_STRING and visit_NUMBER were not touched in this PR. Looking into it cc @hauntsaninja |
Is it OK to just revert that one? |
Pushed a fix for that issue. |
Congratulations, and thank you! Since this fixes such a commonly encountered issue, I'll make a release soon so people can start using it. |
Congratulations y'all, good work! 🖤 |
Description
Adds support for PEP 701: Syntactic formalization of f-strings to black's tokenizer.
Resolves #3746
Given this Python file:
Previous output:
Current output
Checklist - did you ...
CHANGES.md
if necessary?