-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: dedupe marimo dropdown and connection helpers #3
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
Changes from 1 commit
17a32c5
1360cbc
f18450a
eb50371
a18b11c
f9a1ed7
bc4828b
0a11268
c494a18
a48fe3c
a060ade
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| """Shared dropdown option helpers for Marimo UI widgets.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from collections.abc import Callable | ||
| from typing import Any | ||
|
|
||
| import marimo as mo | ||
|
|
||
| from hotdata_runtime import HotdataClient | ||
|
|
||
|
|
||
| def unique_label_options( | ||
| pairs: list[tuple[str, str]], | ||
| *, | ||
| disambiguate: Callable[[str, str, int], str] | None = None, | ||
| ) -> dict[str, str]: | ||
| """Build a label→value map, suffixing repeated labels when needed.""" | ||
| counts: dict[str, int] = {} | ||
| options: dict[str, str] = {} | ||
| for label, value in pairs: | ||
| count = counts.get(label, 0) | ||
| counts[label] = count + 1 | ||
| if count == 0: | ||
| key = label | ||
| elif disambiguate is not None: | ||
| key = disambiguate(label, value, count) | ||
| else: | ||
| key = f"{label} ({count + 1})" | ||
| options[key] = value | ||
| return options | ||
|
|
||
|
|
||
| def empty_dropdown( | ||
| *, | ||
| label: str, | ||
| message: str, | ||
| full_width: bool = True, | ||
| ): | ||
| return mo.ui.dropdown( | ||
| options={message: ""}, | ||
| label=label, | ||
| full_width=full_width, | ||
| ) | ||
|
|
||
|
|
||
| def connection_options(conns: list[Any]) -> dict[str, str]: | ||
| pairs = [(str(c.name), str(c.id)) for c in conns] | ||
| return unique_label_options( | ||
| pairs, | ||
| disambiguate=lambda label, value, count: f"{label} ({value})", | ||
| ) | ||
|
|
||
|
|
||
| def connection_picker( | ||
| client: HotdataClient, | ||
| *, | ||
| label: str = "Connection", | ||
| full_width: bool = True, | ||
| ): | ||
| listing = client.connections().list_connections() | ||
| conns = listing.connections | ||
| if not conns: | ||
| return empty_dropdown( | ||
| label=label, | ||
| message="(no connections)", | ||
| full_width=full_width, | ||
| ) | ||
| return mo.ui.dropdown( | ||
| options=connection_options(conns), | ||
| label=label, | ||
| full_width=full_width, | ||
| ) | ||
|
|
||
|
|
||
| def resolve_connection_picker( | ||
| client: HotdataClient, | ||
| *, | ||
| label: str = "Connection", | ||
| full_width: bool = True, | ||
| ) -> tuple[Any | None, str | None]: | ||
| """Return ``(dropdown_or_none, implicit_connection_id)`` for table browsers.""" | ||
| listing = client.connections().list_connections() | ||
| conns = listing.connections | ||
| if not conns: | ||
| return None, "" | ||
| if len(conns) == 1: | ||
| return None, conns[0].id | ||
| return connection_picker(client, label=label, full_width=full_width), None | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,6 @@ def __init__( | |
| ) -> None: | ||
| super().__init__(connection, engine_name) | ||
| self._connections_cache: list[Any] | None = None | ||
| self._connection_id_cache: dict[str, str] | None = None | ||
|
|
||
| @property | ||
| def source(self) -> str: | ||
|
|
@@ -71,15 +70,12 @@ def _resolve_should_auto_discover( | |
| return True | ||
| return value | ||
|
|
||
| def _connection_ids(self) -> dict[str, str]: | ||
| if self._connection_id_cache is None: | ||
| self._connection_id_cache = { | ||
| str(c.name): str(c.id) for c in self._connections() | ||
| } | ||
| return self._connection_id_cache | ||
|
|
||
| def _connection_id(self, connection_name: str) -> str | None: | ||
| return self._connection_ids().get(connection_name) | ||
| try: | ||
| return self._connection.connection_id_by_name().get(connection_name) | ||
| except RuntimeError as e: | ||
| LOGGER.warning("%s", e) | ||
| return None | ||
|
Comment on lines
73
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this drops the local |
||
|
|
||
| def _connections(self) -> list[Any]: | ||
| if self._connections_cache is None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,39 +6,11 @@ | |
|
|
||
| from hotdata_runtime import HotdataClient | ||
|
|
||
|
|
||
| def _connection_options(conns: list[Any]) -> dict[str, str]: | ||
| counts: dict[str, int] = {} | ||
| options: dict[str, str] = {} | ||
| for c in conns: | ||
| label = c.name | ||
| count = counts.get(label, 0) | ||
| counts[label] = count + 1 | ||
| key = label if count == 0 else f"{label} ({c.id})" | ||
| options[key] = c.id | ||
| return options | ||
|
|
||
|
|
||
| def connection_picker( | ||
| client: HotdataClient, | ||
| *, | ||
| label: str = "Connection", | ||
| full_width: bool = True, | ||
| ): | ||
| listing = client.connections().list_connections() | ||
| conns = listing.connections | ||
| if not conns: | ||
| return mo.ui.dropdown( | ||
| options={"(no connections)": ""}, | ||
| label=label, | ||
| full_width=full_width, | ||
| ) | ||
| options = _connection_options(conns) | ||
| return mo.ui.dropdown( | ||
| options=options, | ||
| label=label, | ||
| full_width=full_width, | ||
| ) | ||
| from hotdata_marimo._options import ( | ||
| connection_picker, | ||
| empty_dropdown, | ||
| resolve_connection_picker, | ||
| ) | ||
|
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super nit: |
||
|
|
||
|
|
||
| class TableBrowser: | ||
|
|
@@ -66,44 +38,40 @@ def __init__( | |
| self._implicit_connection_id: str | None = None | ||
|
|
||
| if self._override_connection_id is None: | ||
| listing = client.connections().list_connections() | ||
| conns = listing.connections | ||
| if len(conns) > 1: | ||
| self._conn_pick = connection_picker(client) | ||
| elif len(conns) == 1: | ||
| self._implicit_connection_id = conns[0].id | ||
| else: | ||
| self._implicit_connection_id = "" | ||
| self._conn_pick, self._implicit_connection_id = resolve_connection_picker( | ||
| client | ||
| ) | ||
|
|
||
| self._table_pick_ctx: str | None = None | ||
| self._init_table_pick() | ||
|
|
||
| def _init_table_pick(self) -> None: | ||
| if self._conn_pick is not None: | ||
| self.table_pick = mo.ui.dropdown( | ||
| options={"(select connection above)": ""}, | ||
| self.table_pick = empty_dropdown( | ||
| label="Table", | ||
| full_width=True, | ||
| message="(select connection above)", | ||
| ) | ||
| self._empty_catalog = True | ||
| self._all_names = [] | ||
| return | ||
|
|
||
| names = self._names_for_active_connection() | ||
| self._all_names = names | ||
| if not names: | ||
| self.table_pick = empty_dropdown( | ||
| label="Table", | ||
| message="(no tables in catalog)", | ||
| ) | ||
| self._empty_catalog = True | ||
| self._all_names: list[str] = [] | ||
| else: | ||
| names = self._names_for_active_connection() | ||
| self._all_names = names | ||
| if not names: | ||
| self.table_pick = mo.ui.dropdown( | ||
| options={"(no tables in catalog)": ""}, | ||
| label="Table", | ||
| full_width=True, | ||
| ) | ||
| self._empty_catalog = True | ||
| else: | ||
| self._empty_catalog = False | ||
| self.table_pick = mo.ui.dropdown( | ||
| options={n: n for n in names}, | ||
| label="Table", | ||
| full_width=True, | ||
| searchable=True, | ||
| ) | ||
| self._table_pick_ctx = self._active_connection_id() | ||
| self._empty_catalog = False | ||
| self.table_pick = mo.ui.dropdown( | ||
| options={n: n for n in names}, | ||
| label="Table", | ||
| full_width=True, | ||
| searchable=True, | ||
| ) | ||
| self._table_pick_ctx = self._active_connection_id() | ||
|
|
||
| def _active_connection_id(self) -> str | None: | ||
| if self._override_connection_id is not None: | ||
|
|
@@ -128,10 +96,9 @@ def _rebuild_table_pick(self, names: list[str]) -> None: | |
| self._all_names = names | ||
| if not names: | ||
| self._empty_catalog = True | ||
| self.table_pick = mo.ui.dropdown( | ||
| options={"(no tables in catalog)": ""}, | ||
| self.table_pick = empty_dropdown( | ||
| label="Table", | ||
| full_width=True, | ||
| message="(no tables in catalog)", | ||
| ) | ||
| else: | ||
| self._empty_catalog = False | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: when
len(conns) > 1,resolve_connection_pickercallsclient.connections().list_connections()here, then immediately callsconnection_picker(client, ...)which fetches the same listing again. Since you've just fetchedconns, consider passing them through to avoid the duplicate API round-trip (e.g., splitconnection_pickerso the dropdown can be built from a pre-fetchedconnslist). The pre-refactor code had the same duplicate fetch, so this isn't a regression — just an opportunity now that the logic is in one place. (not blocking)