POK-91: Fix pluck's broken file ops and import-time crash#33
Conversation
saheljalal
left a comment
There was a problem hiding this comment.
Review β POK-91: pluck import crash & broken file ops
Verdict: Approve, with one should-fix. The diagnosis is right and the three root causes are real, correctly fixed, and I reproduced the before/after behavior in a tomli_w-absent venv (the POK-84 environment).
Verified
- Lazy
tomli_wβ module now imports clean without the dep (import OK, version 1.1.1,tomli_w is None True). The characterization loader no longer caches a half-initialized module. β HANDLERSβFORMAT_HANDLERSsplit (pluck:588,pluck:597) βhandler_fornow indexes the format map; the command-dispatchHANDLERSatpluck:1106is distinct. JSON/envsetround-trips work end to end. β- str/bytes fix (
pluck:546,pluck:549,pluck:707) βtomli_w.dumps()returnsstr;write_text(..., encoding="utf-8")and.rstrip()onstrare correct. β - Write boundary you flagged β no write path assumes the dep at import.
pluck set config.toml β¦withouttomli-wfails with a cleanPluckError(tomli-w is required to write TOML), rc=1, no traceback. β
Should-fix
pluck:706(show_value, TOML branch) β the default (non---json)pluck get config.toml keycallstoml_writer().dumps(value), so a pure read requires the write dep. In the exacttomli-w-absent venv this issue came from,pluck get config.toml web.porterrors instead of printing8080, while--jsonworks and the file load itself works. This contradicts the PR's own "reading TOML uses stdlibtomllib; only writing needstomli_w" framing.- Why it matters:
getis the most common read command; erroring on the default display for a supported read is surprising, and it's the precise scenario POK-91 targets. - Suggested fix: when
tomli_w is None, degrade the TOML display to a writer-free rendering (reuse the JSON pretty-print, orstr(value)) instead of raising β reads should never need the writer.
- Why it matters:
Nit (optional)
- The new
test_pluck_get_reads_valueexercises only--json, so the default-display path above stays untested. A default-display assertion (or the fallback fix) would pin the read-without-writer path that this PR is fundamentally about. - Pre-existing, out of scope:
EnvHandler.save(pluck:569β573) has anif/elsewhose two branches callset_keyidentically β dead branching, harmless. Not introduced here; noting only.
Nice work closing the test gap that let bugs #2/#3 ship invisibly β the dispatch β handler_for β load/save round-trips are exactly the coverage that was missing. CHANGELOG and the 1.1.0 β 1.1.1 PATCH bump are accurate.
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 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
POK-91: Fix pluck's broken file ops and import-time crash
|
Addressed in 164f996 (approved review's should-fix + nit). Should-fix β Nit β untested default display: extended EnvHandler dead branch: left as-is per your "pre-existing / out of scope / noting only". Full suite 408 passed; CI green on 164f996. There were no inline review threads to mark resolved (the review is a single review body), so noting resolution here. |
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 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
164f996 to
690c89f
Compare
Summary
pluck's characterization suite was failing (9 tests, POK-91), but that was only the visible tip of a tool that was non-functional across the board. This restorespluckto working order and adds tests that actually cover the tool's core path.Three independent, committed-on-
mainbugs, all fixed here:Import-time crash (the reported symptom).
import tomli_wwas unguarded, so when the optional write-side dep was absent the entire module failed to import β taking down every unrelated command and, via the shared test loader, reporting all 9 pluck characterization tests as failing (AttributeError: module '_tool_pluck' has no attribute 'parse_path', etc.). This is exactly what happened in the POK-84 aikit QA venv (base + aikit deps +ruamel.yaml, but notomli-w). Fixed by importingtomli_wlazily likeruamel.yaml: reading TOML uses stdlibtomllib, and writing TOML withouttomli-wnow raises a cleanPluckErrorinstead of crashing.HANDLERSname collision β every file op broken. The command-dispatch dictHANDLERS(added when pluck was refactored onto scriptkit'ssk.dispatchin1f2b7a06) shadowed the pre-existing format-handler map of the same name. Sohandler_for()looked aConfigFormatenum up in the command table and everyget/set/copy/merge/diffcrashed with an unhandledKeyErroron all four formats. No test caught this. Renamed the format map toFORMAT_HANDLERS.TOML write str/bytes mismatch.
tomli_w.dumps()returnsstr, butsave()/dump_preview()/thegetdisplay path treated it asbytes(path.write_bytes(...)/.decode("utf-8")), so any TOML write or preview raisedTypeError/AttributeError. Fixed to write text.Why the suite didn't catch bugs 2 & 3
The characterization tests pinned only pure helpers +
--helpβ never thedispatch β handler_for β load/savepath, which is the whole tool. This PR adds end-to-endget/setround-trip tests across all four formats. They fail on the pre-fix tool (verified by revertingpluckand re-running) and pass after.Verification
get/set/copy/diff/doctor/formats/--versionall exercised across yaml Β· json Β· toml Β· env, including cross-formatcopy(TOML write).tomli-w,--helpand the characterization suite still pass, TOML read works, and TOML write fails with a cleanβ tomli-w is required to write TOML (pip install tomli-w)(no traceback).Bumps
pluck1.1.0 β 1.1.1 (PATCH β bug fixes, no interface change) with a CHANGELOG entry.Closes POK-91
π€ Generated with Claude Code