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

Object proxy #121

Merged
merged 16 commits into from
Jan 9, 2024
Merged

Object proxy #121

merged 16 commits into from
Jan 9, 2024

Conversation

Korijn
Copy link
Collaborator

@Korijn Korijn commented Jan 5, 2024

Closes #103

I think I got it down pretty nicely this time.

It required a very different approach, and a way to distinguish stateful attributes from callables.

@Korijn Korijn marked this pull request as ready for review January 5, 2024 13:52
observ/object_proxy.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@berendkleinhaneveld berendkleinhaneveld left a comment

Choose a reason for hiding this comment

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

Nice surprise to see that you've managed to tackle this!

Would love to play around with this. I guess the test suite might need some more love, but that can also wait until we've tried it out for a bit.

Would be nice to see an update to the benchmark to get a sense of how it affects performance.

Just came to mind: does the to_raw method still work? Or did I miss that in the review?

observ/object_utils.py Show resolved Hide resolved
observ/proxy.py Show resolved Hide resolved
observ/watcher.py Show resolved Hide resolved
observ/object_proxy.py Show resolved Hide resolved
observ/object_utils.py Outdated Show resolved Hide resolved
@Korijn
Copy link
Collaborator Author

Korijn commented Jan 5, 2024

Just came to mind: does the to_raw method still work? Or did I miss that in the review?

Not for objects. I can't think of a way to implement it.

@Korijn
Copy link
Collaborator Author

Korijn commented Jan 6, 2024

Nice surprise to see that you've managed to tackle this!

I am very relieved that I can finally put this to rest. It's been in the back of my mind for years. 🤣

I have to say I am surprised this works as well as it does. It's still technically possible to circumvent the mechanisms here, but it requires very specific, outlandish setups.

For example, ndarrays are objects but I do not think they will be reactive. We have to test I guess.

@Korijn
Copy link
Collaborator Author

Korijn commented Jan 8, 2024

For example, ndarrays are objects but I do not think they will be reactive. We have to test I guess.

ndarrays (and memoryviews) are definitely not reactive. We would have to write a separate ndarray_proxy.py module for that and I would not recommend that anybody use it. :) So I extended the type_test to exclude ndarray's from proxying.

@Korijn Korijn merged commit a101d95 into master Jan 9, 2024
7 checks passed
@Korijn Korijn deleted the object-proxy branch January 9, 2024 16:52
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.

Support for dataclasses and / or objects with simple attributes
2 participants