-
-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: improve peer list display #9440
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
Open
pjwerneck
wants to merge
8
commits into
dev
Choose a base branch
from
pjwerneck/improve-peer-list-display
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+238
−40
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2bc561a
Fix JobRef import path
pjwerneck 2ca5bec
feat: improve peer list display
pjwerneck 2c49660
feat: add tests for PeerList summary rendering and HTML representation
pjwerneck a601283
feat: modify PeerList to use composition
pjwerneck 6a7ab3c
fix: ensure PeerList initialization is backwards compatible
pjwerneck 856cb99
fix: reduce traceback noise
pjwerneck 6a16cef
fix: env-var should be treated as boolean
pjwerneck a7c98ef
fix: fix PeerList initialization
pjwerneck File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,57 +1,108 @@ | ||
| import html | ||
| import os | ||
| from collections.abc import Iterator | ||
|
|
||
| from syft_client.sync.peers.peer import Peer, PeerState | ||
| from syft_client.sync.reprs.peer_repr import get_peer_list_table | ||
|
|
||
| # Emoji + friendly label shown per peer state in the summary view. | ||
| _STATE_DISPLAY = { | ||
| PeerState.ACCEPTED: ("✅", "connected"), | ||
| PeerState.REQUESTED_BY_ME: ("⏳", "requested_by_us (waiting for approval)"), | ||
| PeerState.REQUESTED_BY_PEER: ( | ||
| "📩", | ||
| "requested_by_peer (waiting for your approval)", | ||
| ), | ||
| PeerState.REJECTED: ("❌", "rejected"), | ||
| } | ||
|
|
||
| # Display order: accepted first, then outgoing requests, then incoming requests. | ||
| _STATE_ORDER = { | ||
| PeerState.ACCEPTED: 0, | ||
| PeerState.REQUESTED_BY_ME: 1, | ||
| PeerState.REQUESTED_BY_PEER: 2, | ||
| } | ||
|
|
||
|
|
||
| def _peer_sort_key(peer: Peer) -> int: | ||
| return _STATE_ORDER.get(peer.state, 3) | ||
|
|
||
| class PeerList(list): | ||
| def __init__(self, *args: Peer, **kwargs): | ||
|
|
||
| class PeerList: | ||
| """A list-like container for Peer objects with a friendly summary display.""" | ||
|
|
||
| def __init__(self, peers: list[Peer] | None = None) -> None: | ||
| """ | ||
| PeerList is a list specifically for Peer objects. | ||
| Validates that all items are Peer objects and that they are sorted correctly. | ||
| Validates that all items are Peer objects, then sorts them for display: | ||
| accepted first, then requested_by_me, then requested_by_peer. | ||
| """ | ||
| super().__init__(*args, **kwargs) | ||
| # Validate all items are Peer objects | ||
| for item in self: | ||
| peers = list(peers) if peers else [] | ||
| for item in peers: | ||
| if not isinstance(item, Peer): | ||
| raise TypeError( | ||
| f"All items in PeerList must be Peer objects, but got {type(item)}" | ||
| ) | ||
| # Validate sorting (approved before pending) | ||
| self._validate_sorting() | ||
|
|
||
| def _validate_sorting(self): | ||
| """Ensure peers are sorted: accepted, then requested_by_me, then requested_by_peer""" | ||
| order = { | ||
| PeerState.ACCEPTED: 0, | ||
| PeerState.REQUESTED_BY_ME: 1, | ||
| PeerState.REQUESTED_BY_PEER: 2, | ||
| } | ||
| last_order = -1 | ||
| for peer in self: | ||
| peer_order = order.get(peer.state, 3) | ||
| if peer_order < last_order: | ||
| raise ValueError( | ||
| "PeerList must be sorted: accepted first, then requested_by_me, then requested_by_peer" | ||
| ) | ||
| last_order = peer_order | ||
| # ensure consistent ordering for display | ||
| self._peers: list[Peer] = sorted(peers, key=_peer_sort_key) | ||
|
|
||
| def __len__(self) -> int: | ||
| return len(self._peers) | ||
|
|
||
| def __iter__(self) -> Iterator[Peer]: | ||
| return iter(self._peers) | ||
|
|
||
| def __getitem__(self, index: str | int) -> Peer: | ||
| if isinstance(index, int): | ||
| return super().__getitem__(index) | ||
| return self._peers[index] | ||
| elif isinstance(index, str): | ||
| key = index | ||
| for peer in self: | ||
| if peer.email == key: | ||
| return peer | ||
| raise ValueError(f"Peer with email {index} not found") | ||
| try: | ||
| return next(peer for peer in self._peers if peer.email == index) | ||
| except StopIteration: | ||
| raise ValueError(f"Peer with email {index} not found") from None | ||
| else: | ||
| raise ValueError(f"Invalid index type: {type(index)}") | ||
| raise TypeError(f"Invalid index type: {type(index)}") | ||
|
|
||
| def _repr_html_(self) -> str: | ||
| """Used by Jupyter to display Rich HTML.""" | ||
| peers = [p for p in self] | ||
| return get_peer_list_table(peers) | ||
| def _summary_text(self) -> str: | ||
| """Clean, human-friendly summary of the peers and what to do next.""" | ||
| if not self: | ||
| return "👥 You have no peers yet." | ||
|
|
||
| lines = [f"👥 Your peers ({len(self)}):", ""] | ||
| pad = max(len(peer.email) for peer in self) | ||
| connected = [] | ||
| has_pending = False | ||
| for peer in self: | ||
| emoji, label = _STATE_DISPLAY.get(peer.state, ("❓", str(peer.state.value))) | ||
| lines.append(f" {emoji} {peer.email.ljust(pad)} — {label}") | ||
| if peer.state == PeerState.ACCEPTED: | ||
| connected.append(peer) | ||
| elif peer.state in (PeerState.REQUESTED_BY_ME, PeerState.REQUESTED_BY_PEER): | ||
| has_pending = True | ||
|
|
||
| if connected: | ||
| lines += [ | ||
| "", | ||
| "💡 Tip: Once connected, you can access their datasets with:", | ||
| f' client.datasets.get("dataset_name", datasite="{connected[0].email}")', | ||
| ] | ||
| if has_pending: | ||
| lines += [ | ||
| "", | ||
| "⏳ You have pending requests waiting for approval — " | ||
| "follow up with the peer or check back later.", | ||
| ] | ||
| return "\n".join(lines) | ||
|
|
||
| def __str__(self) -> str: | ||
| """Clean summary shown by print() and outside notebooks.""" | ||
| return self._summary_text() | ||
|
|
||
| def _repr_html_(self) -> str | None: | ||
| """Used by Jupyter to display the summary; falls back to text elsewhere.""" | ||
| if os.environ.get("SYFT_NO_REPR_HTML", "").lower() in {"1", "true", "yes"}: | ||
| return None | ||
| return f"<pre>{html.escape(self._summary_text())}</pre>" | ||
|
|
||
| def __repr__(self): | ||
| """Fallback for normal REPL""" | ||
| """Technical repr for debugging / normal REPL.""" | ||
| peers = [p for p in self] | ||
| return f"PeerList({peers!r})" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| """Tests for PeerList's human-friendly summary rendering (__str__/_repr_html_/__repr__).""" | ||
|
|
||
| import pytest | ||
| from syft_client.sync.peers.peer import Peer, PeerState | ||
| from syft_client.sync.peers.peer_list import PeerList | ||
|
|
||
|
|
||
| def _mixed_peer_list() -> PeerList: | ||
| """A PeerList with one peer in each visible state, in required sort order.""" | ||
| return PeerList( | ||
| [ | ||
| Peer(email="do@org.com", state=PeerState.ACCEPTED), | ||
| Peer(email="other@org.com", state=PeerState.REQUESTED_BY_ME), | ||
| Peer(email="incoming@org.com", state=PeerState.REQUESTED_BY_PEER), | ||
| ] | ||
| ) | ||
|
|
||
|
|
||
| def test_str_header_shows_count(): | ||
| pl = _mixed_peer_list() | ||
| assert str(pl).startswith("👥 Your peers (3):") | ||
|
|
||
|
|
||
| def test_str_renders_emoji_and_friendly_labels(): | ||
| text = str(_mixed_peer_list()) | ||
| assert "✅ do@org.com" in text | ||
| assert "— connected" in text | ||
| assert "⏳ other@org.com" in text | ||
| assert "requested_by_us (waiting for approval)" in text | ||
| assert "📩 incoming@org.com" in text | ||
| assert "requested_by_peer (waiting for your approval)" in text | ||
|
|
||
|
|
||
| def test_connected_tip_present_when_connected_peer_exists(): | ||
| text = str(_mixed_peer_list()) | ||
| assert "client.datasets.get" in text | ||
| assert 'datasite="do@org.com"' in text | ||
|
|
||
|
|
||
| def test_connected_tip_absent_when_no_connected_peer(): | ||
| pl = PeerList( | ||
| [ | ||
| Peer(email="other@org.com", state=PeerState.REQUESTED_BY_ME), | ||
| Peer(email="incoming@org.com", state=PeerState.REQUESTED_BY_PEER), | ||
| ] | ||
| ) | ||
| assert "client.datasets.get" not in str(pl) | ||
|
|
||
|
|
||
| def test_pending_tip_present_when_pending_peer_exists(): | ||
| text = str(_mixed_peer_list()) | ||
| assert "pending requests" in text | ||
|
|
||
|
|
||
| def test_pending_tip_absent_when_only_connected(): | ||
| pl = PeerList([Peer(email="do@org.com", state=PeerState.ACCEPTED)]) | ||
| assert "pending requests" not in str(pl) | ||
|
|
||
|
|
||
| def test_empty_peer_list_message(): | ||
| assert str(PeerList([])) == "👥 You have no peers yet." | ||
|
|
||
|
|
||
| def test_repr_html_wraps_and_escapes_summary(): | ||
| html = _mixed_peer_list()._repr_html_() | ||
| assert html is not None | ||
| assert html.startswith("<pre>") | ||
| assert html.endswith("</pre>") | ||
| assert "👥 Your peers (3):" in html | ||
|
|
||
|
|
||
| def test_repr_html_escapes_html_in_email(): | ||
| pl = PeerList([Peer(email="a<b>@org.com", state=PeerState.ACCEPTED)]) | ||
| html = pl._repr_html_() | ||
| assert html is not None | ||
| assert "a<b>@org.com" not in html | ||
| assert "a<b>@org.com" in html | ||
|
|
||
|
|
||
| def test_repr_html_returns_none_when_disabled(monkeypatch): | ||
| monkeypatch.setenv("SYFT_NO_REPR_HTML", "1") | ||
| assert _mixed_peer_list()._repr_html_() is None | ||
|
|
||
|
|
||
| def test_repr_preserves_technical_string(): | ||
| assert repr(_mixed_peer_list()).startswith("PeerList(") | ||
|
|
||
|
|
||
| def test_peer_list_len_and_iter(): | ||
| pl = _mixed_peer_list() | ||
| assert len(pl) == 3 | ||
| assert [p.email for p in pl] == [ | ||
| "do@org.com", | ||
| "other@org.com", | ||
| "incoming@org.com", | ||
| ] | ||
|
|
||
|
|
||
| def test_peer_list_getitem_int(): | ||
| assert _mixed_peer_list()[0].email == "do@org.com" | ||
|
|
||
|
|
||
| def test_peer_list_getitem_str(): | ||
| assert _mixed_peer_list()["other@org.com"].state == PeerState.REQUESTED_BY_ME | ||
|
|
||
|
|
||
| def test_peer_list_getitem_str_not_found(): | ||
| with pytest.raises(ValueError, match="not found"): | ||
| _mixed_peer_list()["nobody@org.com"] | ||
|
|
||
|
|
||
| def test_peer_list_getitem_invalid_type(): | ||
| with pytest.raises(TypeError, match="Invalid index type"): | ||
| _mixed_peer_list()[3.14] # type: ignore[index] | ||
|
|
||
|
|
||
| def test_peer_list_rejects_non_peer_items(): | ||
| with pytest.raises(TypeError): | ||
| PeerList(["not-a-peer"]) # type: ignore[list-item] | ||
|
|
||
|
|
||
| def test_peer_list_sorts_unsorted_input(): | ||
| # Construction sorts by state (accepted → requested_by_me → requested_by_peer) | ||
| # rather than requiring the caller to pre-sort. | ||
| pl = PeerList( | ||
| [ | ||
| Peer(email="incoming@org.com", state=PeerState.REQUESTED_BY_PEER), | ||
| Peer(email="connected@org.com", state=PeerState.ACCEPTED), | ||
| Peer(email="outgoing@org.com", state=PeerState.REQUESTED_BY_ME), | ||
| ] | ||
| ) | ||
| assert [p.state for p in pl] == [ | ||
| PeerState.ACCEPTED, | ||
| PeerState.REQUESTED_BY_ME, | ||
| PeerState.REQUESTED_BY_PEER, | ||
| ] | ||
|
|
||
|
|
||
| def test_peer_list_sort_is_stable_within_a_state(): | ||
| # Peers sharing a state keep their input order (stable sort). | ||
| pl = PeerList( | ||
| [ | ||
| Peer(email="second@org.com", state=PeerState.ACCEPTED), | ||
| Peer(email="first@org.com", state=PeerState.ACCEPTED), | ||
| ] | ||
| ) | ||
| assert [p.email for p in pl] == ["second@org.com", "first@org.com"] |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.