docs: improve README for developer experience#9
Conversation
…tabase scoping Pass database= to client.execute_sql() so queries are scoped to a managed database via the X-Database-Id header (hotdata-runtime>=0.2.1). - HotdataMarimoEngine: add default_database= constructor param, pass to execute() - SqlEditor: add database= constructor param, pass to both execute_sql calls - ManagedDatabaseWriter: use description= kwarg matching ManagedDatabase v0.2.0 API - Fix test_databases_marimo.py syntax error and update assertions
…remove dead code - table_browser: extract _set_table_pick() replacing duplicate _init/_rebuild methods; _sync_table_catalog returns bool so ui drops _rebuilt_table_pick_this_run flag; standardize _active_connection_id to use 'or None' consistently - sql_engine: unregister now restores original engine_to_data_source_connection and resets sentinel so register/unregister/register round-trip works correctly - sql_editor: remove dead 'or ""' on _cached_sql (already guarded by None check above) - workspace_selector: cache HotdataClient, only reconstruct when workspace_id changes
…t param
When options is a dict {label: value}, Marimo validates value= against the
dict keys (labels), not the values. _rebuild_database_pick was passing a
database ID (dict value) which raised ValueError on startup. Now resolves
the label key corresponding to the previously-selected ID instead.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) -> None: | ||
| super().__init__(connection, engine_name) | ||
| self._connections_cache: list[Any] | None = None | ||
| self._default_database = default_database |
There was a problem hiding this comment.
nit: default_database is only honored when users manually instantiate HotdataMarimoEngine. Through register_hotdata_sql_engine() Marimo's registry constructs the engine itself with just (connection, engine_name), so this kwarg never gets a value via the documented flow. Worth either documenting how to inject it (e.g. by registering a pre-built engine) or dropping it until there's a real path. (not blocking)
| return | ||
| options = {db.name: db.name for db in dbs} | ||
| value = current if current in options else next(iter(options)) | ||
| options = {db.description or db.id: db.id for db in dbs} |
There was a problem hiding this comment.
nit: Two managed databases with the same description (or both falling back to the same db.id collision pattern) will silently collapse to one dropdown entry, and the second one becomes unselectable. workspace_selector.py already uses unique_label_options to disambiguate — consider reusing the same helper here (e.g. label as f"{description} ({id})" on collision). (not blocking)
| ``` | ||
|
|
||
| Click **Run on Hotdata** after changing SQL. The editor caches the last successful result so downstream cells do not re-query on every refresh. | ||
| Click **Run on Hotdata** after editing SQL. The editor caches the last successful result so downstream cells don't re-query on every refresh. |
There was a problem hiding this comment.
nit: The PR description says this change "covers scoping SQL to a managed database and controlling result size", but the README never mentions the new database= parameter on hm.sql_editor() / HotdataMarimoEngine, and there's no section on result-size controls. If those features are intentionally part of the same PR, the Quickstart or Managed databases section is a natural place to show hm.sql_editor(client, database="<id>"). (not blocking)
| def test_managed_database_writer_loads_parquet(mock_client): | ||
| mock_client.list_managed_databases.return_value = [ | ||
| ManagedDatabase(id="c1", name="sales", source_type="managed"), | ||
| ManagedDatabase(id="c1", description="sales", default_connection_id="conn_c1"), |
There was a problem hiding this comment.
nit: Downstream in this test, database.value = "sales" is asserted as the argument to load_managed_table. But in production, _rebuild_database_pick builds options={description: id}, so the real dropdown's .value is the id ("c1"), not the description ("sales"). The current test only passes because the dropdown is fully mocked. Consider asserting the id, or adding a test that exercises the real options-dict mapping. (not blocking)
1 participant
Rewrites the README from a developer experience perspective: