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

Add contains, reverse operations for Var #1679

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

martinxu9
Copy link
Contributor

@martinxu9 martinxu9 commented Aug 25, 2023

Summary

  • Add reverse operation for list type Var.
  • Add contains operation for list/tuple/dict/str type Var, overrides __contains__ to raise since this cannot be used to return a Var at the end, it gets converted to boolean.
  • UTs for reverse and contains.
  • Pending: add code snippet/example app

Tests

  • new UTs

Examples

  • foreach renders list of texts in the reverse order
  • text box at the bottom shows a boolean printed as string for whether the user typed input is in the list of words.
class State(rx.State):
    """The app state."""
    words: List[str] = ["Hello", "world", "from", "Reflex!"]
    typed_word: str = ""

def index() -> rx.Component:
    return rx.vstack(
        rx.foreach(State.words.reverse(), lambda word: rx.text(word)),
        rx.input(value=State.typed_word, on_change=State.set_typed_word, style=dict(width="10vw")),
        rx.text("Did I type a word in the above?"),
        rx.text(State.words.contains(State.typed_word).to_string()),
    )
image image

@martinxu9 martinxu9 requested review from masenf and picklelo August 25, 2023 07:49
TypeError: the operation is not supported
"""
raise TypeError(
"'in' operator not supported for Var types, use Var.contains() instead."
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason why to not support the native python in operator? do we lose control over the right hand side or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the return of __contains__ seems to be automatically converted to boolean, so instead of something like a var with {[1,2,3].includes(1)}, we get True (the var being eval as a boolean).

other = Var.create(other)
if types._issubclass(self.type_, Dict):
return BaseVar(
name=f"{self.full_name}.has({other.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.

is there a reason to use self.full_name here instead of just self? the __str__ function selectively uses self.full_name in some situations, otherwise wraps the var, or formats it as a str. is it the wrapping with { and } that's causing the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes did not want the enclosing curly brackets.

reflex/vars.py Outdated
raise TypeError(f"Cannot reverse non-list var {self.full_name}.")

return BaseVar(
name=f"{self.full_name}.reverse()",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this gives me hydration errors when i try it in dev mode because it will reverse the state var in place.

Suggested change
name=f"{self.full_name}.reverse()",
name=f"[...{self.full_name}].reverse()",

(render functions are not allowed to mutate values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏼

reflex/vars.py Outdated
Comment on lines 747 to 748
if not types._issubclass(self.type_, list):
raise TypeError(f"Cannot reverse non-list var {self.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.

when the var's type_ is None, this gives a really confusing error

  File "/Users/masenf/code/reflex-dev/repro-contains/repro_contains/repro_contains.py", line 27, in index
    rx.foreach(rx.vars.BaseVar(name="['foo', 'me', 'you']").reverse(), rx.text),
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/masenf/code/reflex-dev/reflex/reflex/vars.py", line 748, in reverse
    if not types._issubclass(self.type_, list):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/masenf/code/reflex-dev/reflex/reflex/utils/types.py", line 115, in _issubclass
    return cls_check_base == Any or issubclass(cls_base, cls_check_base)
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: issubclass() arg 1 must be a class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks. Added another place to check None first; also added test with var type_ is None

@martinxu9
Copy link
Contributor Author

@picklelo Added some example code

@martinxu9
Copy link
Contributor Author

@masenf new upload incorporating comments and suggestions

@martinxu9 martinxu9 merged commit 2e1aea9 into main Aug 28, 2023
@Alek99 Alek99 deleted the martinxu9/var-op-contain-reverse branch August 30, 2023 17:46
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.

2 participants