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.

Sign up for GitHub

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

feat: Optionally comparing fields in Var.contains, e.g. on rx.Base based types. #3375

Merged
merged 7 commits into from
May 25, 2024
Merged

feat: Optionally comparing fields in Var.contains, e.g. on rx.Base based types. #3375

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d22640c
feat: Optionally comparing fields, e.g. on rx.Base based types.
abulvenz May 22, 2024
fede69f
feat: Minimally invasive change.
abulvenz May 22, 2024
25e11f8
fix: Supporting old-school python versions.
abulvenz May 22, 2024
d2b3982
Merge remote-tracking branch 'upstream/main' into contains-optionally…
abulvenz May 23, 2024
acaf31b
Merge remote-tracking branch 'upstream/main' into contains-optionally…
abulvenz May 24, 2024
9b22686
fix: Adding masenf's suggestions to use var instead of string.
abulvenz May 24, 2024
bdd1434
Merge remote-tracking branch 'upstream/main' into contains-optionally…
abulvenz May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions reflex/vars.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,19 +838,19 @@ def get_operand_full_name(operand):
if invoke_fn:
# invoke the function on left operand.
operation_name = (
f"{left_operand_full_name}.{fn}({right_operand_full_name})" # type: ignore
)
f"{left_operand_full_name}.{fn}({right_operand_full_name})"
) # type: ignore
else:
# pass the operands as arguments to the function.
operation_name = (
f"{left_operand_full_name} {op} {right_operand_full_name}" # type: ignore
)
f"{left_operand_full_name} {op} {right_operand_full_name}"
) # type: ignore
operation_name = f"{fn}({operation_name})"
else:
# apply operator to operands (left operand <operator> right_operand)
operation_name = (
f"{left_operand_full_name} {op} {right_operand_full_name}" # type: ignore
)
f"{left_operand_full_name} {op} {right_operand_full_name}"
) # type: ignore
operation_name = format.wrap(operation_name, "(")
else:
# apply operator to left operand (<operator> left_operand)
Expand Down Expand Up @@ -1353,11 +1353,12 @@ def __contains__(self, _: Any) -> Var:
"'in' operator not supported for Var types, use Var.contains() instead."
)

def contains(self, other: Any) -> Var:
def contains(self, other: Any, field: Union[str, None] = None) -> Var:
"""Check if a var contains the object `other`.

Args:
other: The object to check.
field: Optionally specify a field to check on both object and the other var.

Raises:
VarTypeError: If the var is not a valid type: dict, list, tuple or str.
Expand Down Expand Up @@ -1393,8 +1394,15 @@ def contains(self, other: Any) -> Var:
raise VarTypeError(
f"'in <string>' requires string as left operand, not {other._var_type}"
)

_var_name = (
f"{self._var_name}.includes({other._var_full_name})"
if field is None
else f"{self._var_name}.some(e=>e.{field}==={other._var_full_name})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this should work if the field is specified as a state var, so maybe we can write it like

field = Var.create_safe(field, _var_is_string=isinstance(field, str))

and

f"{self._var_name}.some(e=>e[{field._var_full_name}]==={other._var_full_name})"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masenf Thanks for your idea. I tried it as you suggested:

    _var_name = None
    if field is None:
        _var_name = f"{self._var_name}.includes({other._var_full_name})"
    else:
        field = Var.create_safe(field, _var_is_string=isinstance(field, str))
        _var_name = f"{self._var_name}.some(e=>e[{field._var_full_name}]==={other._var_full_name})"

The code results in this for string literals passed in

<RadixThemesCheckbox 
  checked={state__state.selected_items.some(e=>e[key]===item.key)} 
 ...
/>

where key is not defined ⚡.
Maybe it would be helpful for me to understand the aim of your suggested change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the purpose is being able to write something like

rx.foreach(
    State.available_items,
    lambda item: rx.menu.item(
        rx.checkbox(
            item.name,
            checked=State.selected_items.contains(item.key, State.filter_key),
            on_change=lambda _: State.select_item(item),
        )
    ),
)

where you can change the value of filter_key dynamically

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah okay, we actually need _var_name_unwrapped, like this

_var_name = f"{self._var_name}.some(e=>e[{field._var_name_unwrapped}]==={other._var_full_name})"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @masenf, I will use your corrected version, it works with both strings and vars.

One thing that is that e.{field} could also take nested values like field="address.street" which will not work with the e[...] syntax.

I can also support this case, when I use this abomination:

_var_name = f"{self._var_name}.some(e=>{field._var_name_unwrapped}.split(\".\").reduce((acc,v)=>acc[v],e)==={other._var_full_name})"
class ComplexKey(rx.Base):
    part1: str
    part2: int

class ComplexItem(rx.Base):
    name: str
    color: str
    key: ComplexKey

Then you could write:

State.selected_items.contains(item.key.part1, "key.part1"),

But at some point it becomes ridiculously complex...

-------- 8< --------- 8< --------------

And ideas for sleepless nights 🛌 🖥️
Please nobody take this seriously 😄

import reflex.js as js
...
checked=State.selected_items.some(
    js.λ("e", 
        js.var("e")
            .subscript(State.filter_key)                        
            .equals(js.var(State.search_text)
        )
    )
),
...
assert js.λ("e", 
        js.var("e")
            .subscript(State.filter_key)                        
            .equals(State.search_text)
        )
    ).__str__() == "e=>e[state__state.filter_key]===state__state.search_text"

assert is_turing_complete(js)

Copy link
Collaborator

Choose a reason for hiding this comment

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

🏆

)

return self._replace(
_var_name=f"{self._var_name}.includes({other._var_full_name})",
_var_name=_var_name,
_var_type=bool,
_var_is_string=False,
merge_var_data=other._var_data,
Expand Down
1 change: 1 addition & 0 deletions tests/test_var.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ def test_str_contains(var, expected):
other_var = BaseVar(_var_name="other", _var_type=str)
assert str(var.contains(other_state_var)) == f"{{{expected}.includes(state.other)}}"
assert str(var.contains(other_var)) == f"{{{expected}.includes(other)}}"
assert str(var.contains("1", "hello")) == f'{{{expected}.some(e=>e.hello==="1")}}'


@pytest.mark.parametrize(
Expand Down
Loading