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

Fix type propagation in ToStringOperation #3895

Merged
merged 12 commits into from
Sep 11, 2024
Merged

Conversation

abulvenz
Copy link
Contributor

@abulvenz abulvenz commented Sep 5, 2024

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Does your submission pass the tests?

closes #3890

reflex/ivars/base.py Outdated Show resolved Hide resolved
Co-authored-by: Masen Furer <m_github@0x26.net>
@abulvenz
Copy link
Contributor Author

abulvenz commented Sep 5, 2024

@benedikt-bartscher I haven't had a look into our PositiveInt class so far. Maybe ToNumberOperation can also be enhanced by a similar change.

@benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher I haven't had a look into our PositiveInt class so far. Maybe ToNumberOperation can also be enhanced by a similar change.

I see no good reason to not propagate the specialized type for ints as well

@abulvenz
Copy link
Contributor Author

abulvenz commented Sep 6, 2024

When looking into NumberOperations I found this asymmetry. Maybe that would be an even better fix?
@benedikt-bartscher @masenf Can somebody judge this?

        if issubclass(fixed_type, bool):
            return self.to(BooleanVar, self._var_type)
        if issubclass(fixed_type, (int, float)):
            return self.to(NumberVar, self._var_type)
        if issubclass(fixed_type, dict):
            return self.to(ObjectVar, self._var_type)
        if issubclass(fixed_type, (list, tuple, set)):
            return self.to(ArrayVar, self._var_type)
        if issubclass(fixed_type, str):
            return self.to(StringVar)   # var_type not propagated here !!!
        if issubclass(fixed_type, Base):
            return self.to(ObjectVar, self._var_type)
        return self

masenf and others added 5 commits September 6, 2024 15:48
When emitting a state update, restore `_self_mutable` to the value it had
previously so that `yield` in the middle of `async with self` does not result
in an immutable StateProxy.

Fix reflex-dev#3869
If a component in the markdown component_map contains children components, use
`_get_all_imports` to recursively enumerate them.

Fix reflex-dev#3880
@abulvenz abulvenz marked this pull request as ready for review September 6, 2024 13:54
@benedikt-bartscher
Copy link
Contributor

When looking into NumberOperations I found this asymmetry. Maybe that would be an even better fix? @benedikt-bartscher @masenf Can somebody judge this?

        if issubclass(fixed_type, bool):
            return self.to(BooleanVar, self._var_type)
        if issubclass(fixed_type, (int, float)):
            return self.to(NumberVar, self._var_type)
        if issubclass(fixed_type, dict):
            return self.to(ObjectVar, self._var_type)
        if issubclass(fixed_type, (list, tuple, set)):
            return self.to(ArrayVar, self._var_type)
        if issubclass(fixed_type, str):
            return self.to(StringVar)   # var_type not propagated here !!!
        if issubclass(fixed_type, Base):
            return self.to(ObjectVar, self._var_type)
        return self

I am not (yet) deep into the new var system, but that sounds like a better approach, and it seems like you already migrated to it?

@abulvenz
Copy link
Contributor Author

Mmh, locally the integration-app-harness tests are passing.

@benedikt-bartscher
Copy link
Contributor

Mmh, locally the integration-app-harness tests are passing.

you need to merge main, fixed in #3902

reflex/ivars/sequence.py Outdated Show resolved Hide resolved
@masenf masenf merged commit 95631ff into reflex-dev:main Sep 11, 2024
46 checks passed
@abulvenz
Copy link
Contributor Author

Thanks @masenf and @benedikt-bartscher. Now I can sleep peacefully 😃

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.

ImmutableVar.guess_type() hides the original var type on fields derived from integral types
3 participants