Add and use pc.list to have list mutation detection#339
Add and use pc.list to have list mutation detection#339picklelo merged 10 commits intoreflex-dev:mainfrom
pc.list to have list mutation detection#339Conversation
pynecone/var.py
Outdated
| return Any | ||
|
|
||
|
|
||
| class PcList(list): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| So that mutation to them can be detected by the app: | ||
| * list | ||
| """ | ||
| for field in self.base_vars.values(): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pynecone/state.py
Outdated
| * list | ||
| """ | ||
| for field in self.base_vars.values(): | ||
| if field.type_ is list: |
There was a problem hiding this comment.
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+
There was a problem hiding this comment.
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/var.py
Outdated
| return Any | ||
|
|
||
|
|
||
| class PcList(list): |
| """ | ||
| self._reassign_field = reassign_field | ||
|
|
||
| super().__init__(original_list) |
There was a problem hiding this comment.
Put the super call first?
pynecone/var.py
Outdated
|
|
||
| def __init__( | ||
| self, | ||
| original_list: list, |
pynecone/state.py
Outdated
| * list | ||
| """ | ||
| for field in self.base_vars.values(): | ||
| if field.type_ is list: |
There was a problem hiding this comment.
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.
| So that mutation to them can be detected by the app: | ||
| * list | ||
| """ | ||
| for field in self.base_vars.values(): |
There was a problem hiding this comment.
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.
| value = getattr(self, field.name) | ||
|
|
||
| value_in_pc_data = _convert_mutable_datatypes( | ||
| value, self._reassign_field, field.name |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Sorry for the delay in reviewing this, will get to it soon! |
picklelo
left a comment
picklelo
left a comment
There was a problem hiding this comment.
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 :)
| value = getattr(self, field.name) | ||
|
|
||
| value_in_pc_data = _convert_mutable_datatypes( | ||
| value, self._reassign_field, field.name |
There was a problem hiding this comment.
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.
| value, self._reassign_field, field.name | ||
| ) | ||
|
|
||
| if utils._issubclass(field.type_, List): |
There was a problem hiding this comment.
Should we create the value_in_pc_data inside this if statement? Since it's not used outside of it
3 participants
To add and use
pc.list/PcListto have list mutation detection.It's not as straightforward as I thought. Or is my approach too complicated?
For #334.
pc.dictwould be in another PR.