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

[wip] Pydantic v2 #2751

Draft
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented Feb 28, 2024

@Alek99 Alek99 added enhancement Anything you want improved feature request A feature you wanted added to reflex labels Feb 29, 2024
@benedikt-bartscher benedikt-bartscher force-pushed the pydantic-v2 branch 4 times, most recently from 79c82a0 to 1934f57 Compare March 1, 2024 18:07
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

Review checkpoint 1: Complete ✅

the "model_post_init" in the State was one thing that tripped me up last time i was working in this code.

@@ -271,11 +277,18 @@ def __init__(self, *args, **kwargs):
field_type = EventChain
elif key in props:
# Set the field type.
field_type = fields[key].type_
field_type = fields[key].annotation
Copy link
Collaborator

Choose a reason for hiding this comment

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

in other places in the code annotation seems to be a function? 🤔 not sure if this is incorrect, but leaving myself a bookmark with this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, cs_obj = field.annotation() is there to initialize default values iirc. Maybe pydantic provides smth like get_field_default which handles default and default_factorys for us?

reflex/components/component.py Outdated Show resolved Hide resolved
reflex/components/component.py Outdated Show resolved Hide resolved
reflex/components/component.py Outdated Show resolved Hide resolved
reflex/components/component.py Outdated Show resolved Hide resolved
reflex/components/radix/themes/base.py Outdated Show resolved Hide resolved
reflex/model.py Show resolved Hide resolved
return super().get_value(value.__wrapped__)
return super().get_value(value)

def _get_value(self, value: Any, **kwargs) -> Any:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why a new function? is this just WiP artifacts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_get_value is there to allow unwrapping the MutableProxys in pydantics serialization. There should be a better way to do this.
Funny sidenote: While _get_value is deprecated by pydantic, they somehow seem to rely on it for serialization.

tests/components/forms/test_debounce.py Outdated Show resolved Hide resolved
tests/components/test_tag.py Outdated Show resolved Hide resolved
@benedikt-bartscher benedikt-bartscher force-pushed the pydantic-v2 branch 3 times, most recently from f74f33d to 1dea3b1 Compare March 5, 2024 00:30
@benedikt-bartscher
Copy link
Contributor Author

@masenf should we revisit this? Or do you plan to migrate rx.Base to dataclasses which could make this obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Anything you want improved feature request A feature you wanted added to reflex
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants