Add and use `pc.list` to have list mutation detection #339

TommyDew42 · 2023-01-25T15:21:22Z

To add and use pc.list/PcList to have list mutation detection.

It's not as straightforward as I thought. Or is my approach too complicated?

For #334. pc.dict would be in another PR.

TommyDew42 · 2023-01-25T15:24:13Z

pynecone/var.py

@@ -748,3 +748,54 @@ def type_(self):
        if "return" in self.fget.__annotations__:
            return self.fget.__annotations__["return"]
        return Any
+
+
+class PcList(list):


PcList sounds more reasonable to me than just List cuz it's a list object specifically for pcnecone. IMO, List is appropriate when it is just a list but with more generalised capabilities.

Yup sounds good!

Small nit, but PcList doesn't follow PEP-8 naming convention, which we should defer to when possible. Prefer PCList:

Note: When using acronyms in CapWords, capitalize all the letters of the acronym. Thus HTTPServerError is better than HttpServerError.

Source: PEP-8

Thanks for catching that! fd7c722

pynecone/var.py

picklelo · 2023-01-25T18:47:36Z

pynecone/state.py

+        So that mutation to them can be detected by the app:
+        * list
+        """
+        for field in self.base_vars.values():


This will work for the common use case - one thing we need to think deeper about is how to treat nested types.

For example, a dict with a list value, a subclass of pc.Base with a list field, etc. We may later need to add some sort of recursive inspection and convert everything to PcList. Or state clearly that we only support this use case and the mutable notice still applies for the other case.

Created _convert_mutable_datatypes in e2dbc36 to recursively convert list to PcList. But now only list & dict would be iterated to recursively do the conversion.

picklelo · 2023-01-25T18:48:40Z

pynecone/state.py

+        * list
+        """
+        for field in self.base_vars.values():
+            if field.type_ is list:


We can use utils._issubclass(field.type_, List) here which will work for things like typing.List[int] also.

Also let's use List everywhere because we need to support Python 3.7+

Ahhhhh thanks for this suggestion. I also noticed the field.type_ in unit tests are all the types from typing instead of the native types.

pynecone/state.py

picklelo · 2023-01-25T18:50:02Z

pynecone/var.py

@@ -748,3 +748,54 @@ def type_(self):
        if "return" in self.fget.__annotations__:
            return self.fget.__annotations__["return"]
        return Any
+
+
+class PcList(list):


Yup sounds good!

picklelo · 2023-01-25T18:50:21Z

pynecone/var.py

+        """
+        self._reassign_field = reassign_field
+
+        super().__init__(original_list)


Put the super call first?

pynecone/var.py

picklelo · 2023-01-25T18:51:27Z

pynecone/var.py

+
+    def __init__(
+        self,
+        original_list: list,


Use List[Any] here

TommyDew42 · 2023-01-26T15:18:09Z

pynecone/state.py

+        * list
+        """
+        for field in self.base_vars.values():
+            if field.type_ is list:


Ahhhhh thanks for this suggestion. I also noticed the field.type_ in unit tests are all the types from typing instead of the native types.

TommyDew42 · 2023-01-26T17:28:38Z

pynecone/state.py

+        So that mutation to them can be detected by the app:
+        * list
+        """
+        for field in self.base_vars.values():


Created _convert_mutable_datatypes in e2dbc36 to recursively convert list to PcList. But now only list & dict would be iterated to recursively do the conversion.

TommyDew42 · 2023-01-26T17:33:12Z

pynecone/state.py

+            value = getattr(self, field.name)
+
+            value_in_pc_data = _convert_mutable_datatypes(
+                value, self._reassign_field, field.name


I found sth interesting. The previous way where we use lambda to bind the field.name to self._reassign_field wouldn't work. The field.name would be dynamically evaluated to be the last field.name instead of the field.name in the current iteration.

So let's say you have 3 fields in a state and the last state.name is friends_in_nested_list (i.e. the case in the unit test), the field_name would always be friends_in_nested_list. 😓 I have no choice but to pass both into the class, instead of use lambda to piece them together.

Any ideas for this problem?

Ahh I think @Lendemor was seeing a similar issue in handling the dynamic routing - I think this code may be relevant where he used a factory? https://github.com/pynecone-io/pynecone/blob/5aae6a122d8935aea599b5a9f8bf84622fda9cdb/pynecone/state.py#L291

I'm not entirely sure though may have to give this a deeper look.

pynecone/var.py

picklelo · 2023-01-27T06:56:33Z

Sorry for the delay in reviewing this, will get to it soon!

picklelo

Awesome job! Tested this out on an app locally and it works nicely. May be some edge cases we need to fix in the future, but will go ahead and merge this in now :)

picklelo · 2023-01-28T18:42:14Z

pynecone/state.py

+            value = getattr(self, field.name)
+
+            value_in_pc_data = _convert_mutable_datatypes(
+                value, self._reassign_field, field.name


Ahh I think @Lendemor was seeing a similar issue in handling the dynamic routing - I think this code may be relevant where he used a factory? https://github.com/pynecone-io/pynecone/blob/5aae6a122d8935aea599b5a9f8bf84622fda9cdb/pynecone/state.py#L291

I'm not entirely sure though may have to give this a deeper look.

picklelo · 2023-01-28T18:43:08Z

pynecone/state.py

+                value, self._reassign_field, field.name
+            )
+
+            if utils._issubclass(field.type_, List):


Should we create the value_in_pc_data inside this if statement? Since it's not used outside of it

pynecone/var.py

TommyDew42 · 2023-01-28T19:14:33Z

@picklelo @kbrgl thanks for the reviews.

@picklelo What are the edge cases we haven't covered in this PR? Will have a follow up PR for them.

TommyDew42 added 4 commits January 25, 2023 23:07

add PcList

c964e49

conovert all list fields type to PcList

579833c

unit tests for list mutation detection

f95de4c

simply PcList

7ed7ca5

TommyDew42 commented Jan 25, 2023

View reviewed changes

TommyDew42 added 2 commits January 25, 2023 23:27

use List & Tuple from typing instead of the native type as type hint

b1bc892

please pass cicd

f58db52

picklelo reviewed Jan 25, 2023

View reviewed changes

TommyDew42 added 3 commits January 27, 2023 01:26

support nest list mutation detection

e2dbc36

update unit tests

71848e2

delete _test_event_delta

0b68e4a

TommyDew42 commented Jan 26, 2023

View reviewed changes

PcList -> PCList

fd7c722

picklelo approved these changes Jan 28, 2023

View reviewed changes

picklelo linked an issue Jan 28, 2023 that may be closed by this pull request

Create pc.dict and pc.list #334

Closed

picklelo merged commit d5a76f1 into reflex-dev:main Jan 28, 2023

ACucos1 pushed a commit to ACucos1/rd-pynecone that referenced this pull request Feb 28, 2023

Add pc.list for mutation detection (reflex-dev#339)

0419335

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and use `pc.list` to have list mutation detection #339

Add and use `pc.list` to have list mutation detection #339

TommyDew42 commented Jan 25, 2023

TommyDew42 Jan 25, 2023

picklelo Jan 25, 2023

kbrgl Jan 27, 2023

TommyDew42 Jan 27, 2023

picklelo Jan 25, 2023

TommyDew42 Jan 26, 2023

picklelo Jan 25, 2023

TommyDew42 Jan 26, 2023

picklelo Jan 25, 2023

picklelo Jan 25, 2023

picklelo Jan 25, 2023

TommyDew42 Jan 26, 2023

TommyDew42 Jan 26, 2023

TommyDew42 Jan 26, 2023

picklelo Jan 28, 2023

picklelo commented Jan 27, 2023

picklelo left a comment

picklelo Jan 28, 2023

picklelo Jan 28, 2023

TommyDew42 commented Jan 28, 2023

Add and use pc.list to have list mutation detection #339

Add and use pc.list to have list mutation detection #339

Conversation

TommyDew42 commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picklelo commented Jan 27, 2023

picklelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyDew42 commented Jan 28, 2023

Add and use `pc.list` to have list mutation detection #339

Add and use `pc.list` to have list mutation detection #339