From 690c89f3e9c96c7e6c377759338940dc9ac196eb Mon Sep 17 00:00:00 2001 From: Sahel Jalal Date: Fri, 3 Jul 2026 05:25:44 +0000 Subject: [PATCH] POK-91: Make TOML get/diff display writer-free (review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Code Reviewer's should-fix on PR #33: the default (non-JSON) `pluck get config.toml key` display routed through `show_value`'s TOML branch, which called the tomli-w writer — so a pure TOML *read* failed in the exact tomli-w-absent venv this issue targets, contradicting the "reading needs no writer" guarantee (reading uses stdlib tomllib). `show_value` now renders a TOML value as TOML only when it can (value is a mapping and tomli-w is present) and falls back to JSON otherwise. This also fixes a latent crash the fix surfaced: `tomli_w.dumps()` needs a top-level table, so displaying a bare scalar/list (`get config.toml a.scalar`) raised `AttributeError: 'int' object has no attribute 'items'` even with the writer installed — the round-trip tests missed it by reading via `--json`. Tests: extended the get characterization test to also exercise the default display path (not just `--json`), and added a test that shadows tomli-w as unimportable to pin that a plain TOML `get` still works writer-free (fails on the pre-fix code, passes after). Full suite 408 passed. Coalesced under the existing uncommitted 1.1.1 changelog entry. Co-Authored-By: Claude Opus 4.8 Co-authored-by: multica-agent --- CHANGELOG.md | 7 ++++++ pluck | 14 ++++++++++-- tests/test_tools_characterization.py | 34 +++++++++++++++++++++++++++- 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eb8f36..1ba7033 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -566,6 +566,13 @@ Newest entries on top, within each tool. - Added end-to-end `get`/`set` round-trip characterization tests across all four formats so the dispatch → handler → load/save path is covered (the prior suite pinned only pure helpers + `--help`). +- Made the default `get`/`diff` TOML *display* writer-free: it no longer calls the + `tomli-w` writer on a pure read (which failed in the `tomli-w`-absent venv), and + a bare scalar/list value now renders as JSON instead of crashing (`tomli_w.dumps` + requires a top-level table — `get config.toml a.scalar.path` used to raise). TOML + tables still render as TOML when the writer is present. Pinned the default + (non-`--json`) display path in the characterization tests, including with the + writer absent. ### 1.1.0 — 2026-06-26 - Added the brand emoji 🪶 to the identity banner (`ICON` now wired into the parser). diff --git a/pluck b/pluck index 53e50eb..85903e4 100755 --- a/pluck +++ b/pluck @@ -704,8 +704,18 @@ def show_value(title: str, loaded: LoadedConfig, value: Any) -> None: text = json.dumps(value, indent=2, default=str) lexer = "json" elif loaded.format == ConfigFormat.TOML: - text = toml_writer().dumps(value).rstrip() or "(empty)" - lexer = "toml" + # Render as TOML only when we actually can: a bare scalar/list has no + # standalone TOML representation (a TOML document is always a table), + # and serializing a table back needs the optional tomli-w writer. + # Otherwise fall back to JSON — it needs no writer and reads fine for + # a `get`, so a pure read never requires tomli-w (stdlib tomllib does + # the reading). + if isinstance(value, dict) and tomli_w is not None: + text = tomli_w.dumps(value).rstrip() or "(empty)" + lexer = "toml" + else: + text = json.dumps(value, indent=2, default=str) + lexer = "json" else: text = str(value) lexer = "bash" diff --git a/tests/test_tools_characterization.py b/tests/test_tools_characterization.py index 51ff038..af325f5 100644 --- a/tests/test_tools_characterization.py +++ b/tests/test_tools_characterization.py @@ -140,13 +140,45 @@ def _require_writer(fmt): @pytest.mark.parametrize("fmt", ["json", "env", "yaml", "toml"]) def test_pluck_get_reads_value(tmp_path, fmt): - """`pluck get` resolves a path in every format (reading needs no writer).""" + """`pluck get` resolves a path in every format, via both output modes.""" body, key, expected = _PLUCK_SAMPLES[fmt] cfg = tmp_path / f"config.{fmt}" cfg.write_text(body) + # machine-readable mode: exact value on stdout proc = _run_pluck("get", "--json", str(cfg), key) assert proc.returncode == 0, proc.stderr assert json.loads(proc.stdout) == expected + # default (rich panel) display mode must also succeed and show the value + proc = _run_pluck("get", str(cfg), key) + assert proc.returncode == 0, proc.stderr + assert str(expected) in proc.stdout + + +def test_pluck_get_toml_display_needs_no_writer(tmp_path): + """Default (panel) `get` on TOML must not require the tomli-w *writer*. + + Reading TOML uses stdlib ``tomllib``; only rendering the value back as TOML + needed ``tomli_w``. Simulate the writer's absence by shadowing it with a + module that fails to import, and assert a plain `get` (no ``--json``) still + succeeds — the exact tomli-w-absent venv this issue targets. (``--json`` + renders via ``json.dumps`` and never needed the writer.) + """ + shim = tmp_path / "no_writer" + shim.mkdir() + (shim / "tomli_w.py").write_text('raise ImportError("simulated: tomli-w absent")\n') + cfg = tmp_path / "config.toml" + cfg.write_text('model = "gpt-4o"\n[web]\nport = 8080\n') + env = { + **os.environ, + "NO_COLOR": "1", + "PYTHONPATH": str(shim) + os.pathsep + os.environ.get("PYTHONPATH", ""), + } + proc = subprocess.run( + [sys.executable, str(REPO_ROOT / "pluck"), "get", str(cfg), "web.port"], + capture_output=True, text=True, timeout=60, env=env, + ) + assert proc.returncode == 0, proc.stderr + assert "8080" in proc.stdout @pytest.mark.parametrize("fmt", ["json", "env", "yaml", "toml"])