Skip to content

Conversation

@PythonFZ
Copy link
Member

@PythonFZ PythonFZ commented Jul 15, 2025

  • should the nested refresh message contain the data in this way? Do we even need a message or would it be enough to trigger an event, because the event can gather the data itself?
  • This does not really help, because we need to be able to make a change on the python side and trigger a change on the typescript side. So tracking the instances only on the Python side is not helpful!

@PythonFZ PythonFZ requested a review from Copilot July 15, 2025 06:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for nested refresh callbacks so that parent List/Dict objects receive refresh events when their nested children change. Key changes include:

  • Tracking parent watchers and propagating refresh notifications from child objects
  • Extending on_refresh API to enable nested refresh and registering client instances globally
  • Adding tests for nested refresh behavior in both List and Dict

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
znsocket/objects/list_obj.py Track parent watchers, notify on nested changes, update on_refresh API
znsocket/objects/dict_obj.py Same nested watcher/notification logic for Dict
znsocket/client.py Global client registry and helper to find nested-refresh-enabled clients
tests/test_list.py New test_list_nested_refresh scenario
tests/test_dict.py New test_dict_nested_refresh scenario

Comment on lines 22 to 36
_all_clients: list[weakref.ref] = []


def _get_clients_with_nested_refresh(key: str) -> list["Client"]:
"""Get all client instances that have nested refresh enabled for the given key."""
clients = []
# Clean up dead references
global _all_clients
_all_clients = [ref for ref in _all_clients if ref() is not None]

for client_ref in _all_clients:
client = client_ref()
if (
client
and key in client.nested_refresh_keys
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Use a weakref.WeakSet to track client instances instead of a list of weakrefs and manual filtering; this simplifies cleanup and improves performance.

Suggested change
_all_clients: list[weakref.ref] = []
def _get_clients_with_nested_refresh(key: str) -> list["Client"]:
"""Get all client instances that have nested refresh enabled for the given key."""
clients = []
# Clean up dead references
global _all_clients
_all_clients = [ref for ref in _all_clients if ref() is not None]
for client_ref in _all_clients:
client = client_ref()
if (
client
and key in client.nested_refresh_keys
_all_clients: weakref.WeakSet["Client"] = weakref.WeakSet()
def _get_clients_with_nested_refresh(key: str) -> list["Client"]:
"""Get all client instances that have nested refresh enabled for the given key."""
clients = []
for client in _all_clients:
if (
key in client.nested_refresh_keys

Copilot uses AI. Check for mistakes.
"""Whether nested refresh is enabled for this object."""
return self._nested_refresh

def _add_parent_watcher(
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] The nested-refresh watcher methods are duplicated in both List and Dict; consider extracting this logic into a shared base class or mixin to reduce duplication.

Copilot uses AI. Check for mistakes.
ZnSocketObject,
)
from znsocket.client import Client
from znsocket.client import Client, _get_clients_with_nested_refresh
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Importing a private helper (_get_clients_with_nested_refresh) may break encapsulation; consider making this function part of the public API or renaming it.

Suggested change
from znsocket.client import Client, _get_clients_with_nested_refresh
from znsocket.client import Client, get_clients_with_nested_refresh

Copilot uses AI. Check for mistakes.
@coveralls
Copy link

coveralls commented Jul 15, 2025

Pull Request Test Coverage Report for Build 16286457857

Details

  • 92 of 109 (84.4%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.4%) to 66.604%

Changes Missing Coverage Covered Lines Changed/Added Lines %
znsocket/client.py 8 10 80.0%
znsocket/objects/dict_obj.py 37 43 86.05%
znsocket/objects/list_obj.py 47 56 83.93%
Files with Coverage Reduction New Missed Lines %
znsocket/objects/list_obj.py 6 84.62%
Totals Coverage Status
Change from base Build 16268378295: 1.4%
Covered Lines: 1059
Relevant Lines: 1590

💛 - Coveralls

@PythonFZ PythonFZ added the wontfix This will not be worked on label Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants