Fix KV f-string name extraction for boolean expressions - #9393

Thanks for opening your first pull request here! 💖 Please check out our contributing guidelines.

Here is a simple reproducer:

'''
Edit the box. The plain line follows. The f-string line stays put.
When the text is empty, the plain line shows 'backup' and the f-string line still shows 'hello'.
'''

from kivy.app import App
from kivy.lang import Builder

kv = '''
BoxLayout:
    orientation: 'vertical'
    padding: 16
    spacing: 8
    Label:
        text: 'plain: ' + (source.text or 'backup')
    Label:
        text: 'f-string: ' + f"{source.text or 'backup'}"
    TextInput:
        id: source
        text: 'hello'
        multiline: False
        size_hint_y: None
        height: 40
'''


class Demo(App):
    def build(self):
        return Builder.load_string(kv)


if __name__ == '__main__':
    Demo().run()

Thanks for the fix. The parser change looks right to me. I'd ask for a few changes to the tests so they fail on master and pass only with this fix.

test_fstring_boolop_names

1. Use assert names == expected instead of assert expected <= names. With <=, the test still passes if the walker returns extra names.

2. Add a case with dotted names to cases, which is how KV rules are written:

    f"{original.text or other.text}": {'original.text', 'other.text'},

test_bind_fstring_boolop

1. Add a second label id and a third target that uses it, so the test also checks the name on the right side of or:

    Label:
        id: other
        text: 'backup'
    Label:
        id: target3
        text: f"{original.text or other.text}"

2. Change original.text to a new non-empty value before clearing it. Right now the test only sets it to '', and on master the labels already show 'fallback' and '', so those checks pass without the fix.

The end of the test would look like this:

    assert root.ids.target1.text == 'perfect'
    assert root.ids.target2.text == 'truthy'
    assert root.ids.target3.text == 'perfect'

    root.ids.original.text = 'changed'
    assert root.ids.target1.text == 'changed'
    assert root.ids.target3.text == 'changed'

    root.ids.original.text = ''
    assert root.ids.target1.text == 'fallback'
    assert root.ids.target2.text == ''
    assert root.ids.target3.text == 'backup'

    root.ids.other.text = 'new backup'
    assert root.ids.target3.text == 'new backup'

I checked these against current master and against your change. Every new check fails on master and passes with the fix.

Thanks, I applied your test changes in c6335e0: strict names == expected, the dotted-name case, the second label and third target, and 'changed' before clearing. I also fixed a quoting bug in my test file. The triple quotes in test_bind_fstring_boolop had been pushed with backslashes, which would not parse, so that is repaired too.

What I ran: the names test passes with the dotted case against the new parser. I could not run test_bind_fstring_boolop here (my environment has a mismatched Kivy build, so it fails on style.kv before the test body). Please rely on CI for that one.

Thanks for the updates. I ran the new tests locally: both fail against master's parser and pass with your change, and CI is green. This looks good to me.

A recommended simplification: In parser.py the shared branch can stay if the f-string unwrapping moves into its own FormattedValue branch:

        if isinstance(node, (ast.JoinedStr, ast.BoolOp)):
            for n in node.values:
                yield from cls.get_names_from_expression(n)

        if isinstance(node, ast.FormattedValue):
            yield from cls.get_names_from_expression(node.value)

Literal text in an f-string is a Constant, which yields no names, so the loop no longer needs .value.

In the simpler version, the .value step belongs to FormattedValue, not to the loop. With a branch for it, both lists (ast.JoinedStr, ast.BoolOp) can be walked the same way, as shown above. It's the same behavior with one loop instead of two.

I ran the PR's tests with this version and they pass. The current change is also correct. I suspect the maintainer would prefer the simplified version.

I apologize for not noticing this my initial review.

Thanks for running the tests and for the suggestion. I moved the f-string unwrapping into its own FormattedValue branch as you showed, so JoinedStr and BoolOp share one loop. Pushed in f7cfbb7. I only checked the name extraction on its own (dotted names, plain f-string, or with a literal); I did not run the full KV tests, so I will rely on CI for that.

Thanks for the review, kuzeyron. Is there anything else you need from me before this can merge?

Thanks for the review, kuzeyron. Is there anything else you need from me before this can merge?

Looks good to me thank you!

The backport to devel-2.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-devel-2.3.x devel-2.3.x
# Navigate to the new working tree
cd .worktrees/backport-devel-2.3.x
# Create a new branch
git switch --create backport-9393-to-devel-2.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 951556e09341f0d5a8609cfc20c487a7551ea038
# Push it to GitHub
git push --set-upstream origin backport-9393-to-devel-2.3.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-devel-2.3.x

Then, create a pull request where the base branch is devel-2.3.x and the compare/head branch is backport-9393-to-devel-2.3.x.