fix: disallow_overwrites no-op and cache_func_outputs default/kwargs bugs - #104
Merged
Merged
Conversation
…bugs
- dol.trans.disallow_overwrites: was a no-op (built an inner __setitem__
but never attached it, returned None). Now returns a proper subclass
of `store` with the __setitem__ guard attached, and (per the
`disable_deletes` param, previously ignored) an equivalent __delitem__
guard. error_msg supports both `{}` and `{k}` placeholders.
- dol.caching.cache_func_outputs: with the default `cache=HashableDict`,
`get_cache` returned the *class* (not an instance), so the first call
raised `TypeError: argument of type 'type' is not iterable`. Now
explicitly instantiates a bare class/factory before use. Separately,
kwargs were wrapped in a fresh HashableDict each call, whose __hash__
is identity-based (id(self)) -- so kwargs-based calls never hit the
cache. Now keys on a stable, value-based tuple of the sorted kwargs
items instead.
Both were reported, precisely diagnosed, and left as a fix-or-delete
choice in the issue. Chose "finish" over "delete" (the issue's other
option, and the one #94's cruft audit lists as a candidate) since
they're now correct, useful, and not part of the public dol/__init__.py
API -- so #94 can still relocate them later if fleet adoption stays at
zero.
Reviewed by an independent sub-agent before merge (per crowsnest policy),
which caught two follow-up issues, both fixed here: the error_msg `{k}`
placeholder didn't actually work (positional-only .format call), and
unhashable kwargs values now raise TypeError instead of silently never
caching -- documented as intentional (dol's own "no silent failures"
convention), matching the pre-existing requirement that positional args
be hashable too.
Closes #101
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Fixes the two bugs reported in #101, both found by @thorwhalen while verifying docstring claims for the WP6 documentation sweep:
dol.trans.disallow_overwriteswas a no-op -- it built an inner__setitem__but never attached it and returnedNone. Now returns a proper subclass ofstorewith the__setitem__guard attached, plus (honoringdisable_deletes, previously ignored entirely) an equivalent__delitem__guard.dol.caching.cache_func_outputs: with the defaultcache=HashableDict,get_cachereturned the class rather than an instance (a class object satisfiesis_a_cache'shasattrchecks), so the first call raisedTypeError: argument of type 'type' is not iterable. Fixed by explicitly instantiating a bare class/factory before use. Separately, kwargs were wrapped in a freshHashableDicteach call -- whose__hash__is identity-based (id(self)) -- so kwargs-based calls never hit the cache. Now keyed on a stable, value-based tuple of sorted kwargs items.Chose "finish" over "delete" (the issue's other offered option, and the one #94's cruft audit separately lists as a zero-fleet-usage candidate) since both are now correct and useful, and neither is part of the public
dol/__init__.pysurface -- so #94 can still relocate them later if fleet adoption stays at zero.Closes #101
Review
Per the crowsnest "review before merging behaviour-changing code" policy, this was reviewed by an independent sub-agent before merge (see RUN_LOG). It confirmed both original bugs are fixed with no test regressions, and caught two follow-up issues, both fixed in this PR:
error_msgdocstring claimed{k}worked as a placeholder, but the code did.format(k)(positional-only) --{k}raisedKeyError. Fixed:.format(k, k=k)supports both.TypeErrorinstead of silently never caching (the oldHashableDict-wrapping masked this). Documented as intentional -- matches dol's own "no silent failures" convention, and the pre-existing requirement that positionalargsbe hashable too.Dependents checked
Neither function is exported from
dol/__init__.py, and no on-box fleet package (checked all ~89 dol dependents present under/root/py/proj) imports either by name.Test plan
pytest --doctest-modules dol/trans.py dol/caching.py-> 70 passedpytest(full suite) -> 592 passed, 3 skipped (baseline: 590 passed, 3 skipped before this session's other dol work)🤖 Generated with Claude Code
https://claude.ai/code/session_011HSBVhDjRU4apSLcRkavv9