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 determination of f-string expression spans #2654

Merged
merged 12 commits into from Dec 1, 2021

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Nov 29, 2021

The original goal here was to get rid of the regex dependency, but it turns out the regex failed many of the test cases added in this PR.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Nov 29, 2021

Here's a basic property test (note that the hypothesis.assume means in practice we're unlikely to get valid strings that are particularly long, unless you start playing around with hypothesis.target):

import ast
import hypothesis
from hypothesis import strategies as st


def check_program(program):
    tree = ast.parse(program)

    print(program)

    str_bits = [c.value for c in tree.body[0].value.values if isinstance(c, ast.Constant)]
    expr_spans = list(iter_fexpr_spans(program))
    starts = [a for a, _ in expr_spans]
    ends = [b for _, b in expr_spans]
    expected_bits = [
        x for i, j in zip([2] + ends, starts + [-1])
        if (x := program[i:j].replace("{{", "{").replace("}}", "}"))
    ]
    assert str_bits == expected_bits, (str_bits, expected_bits)


@hypothesis.given(st.text(alphabet="{}' "))
@hypothesis.settings(max_examples=1000, suppress_health_check=[hypothesis.HealthCheck.filter_too_much])
def test_parse_fany(text):
    try:
        check_program(f'f"{text}"')
    except SyntaxError:
        hypothesis.assume(False)


@hypothesis.given(st.text(alphabet="{}' "))
@hypothesis.settings(max_examples=1000, suppress_health_check=[hypothesis.HealthCheck.filter_too_much])
def test_parse_fexpr(text):
    try:
        check_program(f'f"{{{text}}}"')
    except SyntaxError:
        hypothesis.assume(False)

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

For the tests could you add test_trans.py with tests like the ones you list in the PR description.

src/black/trans.py Outdated Show resolved Hide resolved
src/black/trans.py Show resolved Hide resolved
src/black/trans.py Outdated Show resolved Hide resolved
hauntsaninja and others added 4 commits November 29, 2021 11:33
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@hauntsaninja hauntsaninja changed the title Remove regex dependency for determining f-string expression spans Fix determination of f-string expression spans Dec 1, 2021
@hauntsaninja hauntsaninja marked this pull request as ready for review December 1, 2021 06:41
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thank you!

@JelleZijlstra JelleZijlstra merged commit f1813e3 into psf:main Dec 1, 2021
@hauntsaninja hauntsaninja deleted the fpoc branch December 1, 2021 18:02
ichard26 added a commit that referenced this pull request Dec 2, 2021
We were no longer using it since GH-2644 and GH-2654. This should hopefully
make using Black easier to use as there's one less compiled dependency.
The core team also doesn't have to deal with the surprisingly frequent fires
the regex packaging setup goes through.

Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>
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.

None yet

2 participants