fix: new-name always wins when both old and new kwargs passed to args_mapping#198
Conversation
…_mapping Before this fix the dict-comprehension in `_apply_args_mapping` (proxy) and `_build_call_plan` (deprecation) used last-write-wins semantics, so the winner depended on Python dict iteration order — `fn(old=5, new=6)` worked but `fn(new=6, old=5)` silently produced the wrong value. - Snapshot explicit new-name values before remap in both code paths; restore via `result.update(explicit_new)` so new-name always wins regardless of call-site argument order - Covers all four surfaces: callable-target proxy, ARGS_REMAP proxy, callable-target function decorator, ARGS_REMAP function decorator - Adds 9 tests (both argument orderings) across TestDataclassAutoExpand, TestArgsMapping, TestArgumentMapping, and TestArgsRemapMode --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
✅ Ready to approve
The precedence fix is correctly implemented in both code paths and is backed by targeted regression tests; remaining feedback is limited to minor test-name clarity.
Note: this review does not count toward required approvals for merging.
Pull request overview
This PR fixes args_mapping collision semantics so that when callers pass both the old and new kwarg names, the explicitly provided new-name value always takes precedence (independent of call-site kwarg ordering). This removes the previous reliance on dict iteration order in both the proxy and decorator call-planning paths.
Changes:
- Ensure “new-name wins” by snapshotting explicit new-name kwargs before remapping and restoring them after remap in both
_DeprecatedProxy._apply_args_mappingand the decorator call-plan remap logic. - Add regression tests covering both kwarg orderings across proxy ARGS_REMAP, decorator ARGS_REMAP, callable-target forwarding, and dataclass auto-expand surfaces.
File summaries
| File | Description |
|---|---|
src/deprecate/proxy.py |
Makes _apply_args_mapping deterministic by restoring explicitly provided new-name kwargs after remap. |
src/deprecate/deprecation.py |
Applies the same precedence rule in the decorator call-plan kwargs remap path. |
tests/unittests/test_proxy.py |
Adds unit coverage for proxy + dataclass auto-expand collision cases and kwarg ordering. |
tests/integration/test_target_mode.py |
Adds integration coverage for ARGS_REMAP mode when both old and new kwargs are provided. |
tests/integration/test_functions.py |
Adds integration coverage for callable-target forwarding when old/new kwargs collide. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 4
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 new files)
Previous Review Summaries (7 snapshots, latest commit ebf10d8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ebf10d8)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 new files)
Previous review (commit 81cb32b)Status: No Issues Found | Recommendation: Merge All changes in this PR are correct: Implementation (deprecation.py, proxy.py): The arg collision precedence fix correctly ensures the explicit new-name value wins when both old and new names are passed. The stacklevel=3 for the collision UserWarning is correct (direct warn call inside Tests: New tests in Documentation: The "silently discarded" phrasing in Files Reviewed (11 files)
Previous review (commit 90732d4)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)Previous inline comments remain active and were not duplicated:
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous review (commit 8119895)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 32717f1)Status: No Issues Found | Recommendation: Merge OverviewIncremental review of changes since commit No new issues found. The core fix remains correct:
Previous inline comments carry forward unchanged. Files Reviewed (10 files)
Previous review (commit 3603d88)Status: No Issues Found | Recommendation: Merge The incremental changes fix an args_mapping precedence bug where an explicitly provided new-name argument could be overwritten by a remapped old-value. The fix:
Files Reviewed (6 files)
Previous review (commit 0d84fb6)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by nex-n2-pro:free · 760,287 tokens |
When args_mapping maps old→new while also dropping new (e.g. {old:'new', new:None}),
the result.update(explicit_new) call previously reintroduced the key that args_to_drop
had just suppressed. One-token fix at both remap sites: add `and new_k not in
args_to_drop` (proxy) / `and new_k not in args_skip` (deprecation) to the
explicit_new comprehension condition so an explicit drop config always wins.
Also adds a one-sentence precedence note to _apply_args_mapping docstring and
the dep_cfg param description in _build_call_plan.
---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…xtra priority collision tests
Cover three scenarios the PR's existing test suite left untested:
- Callable-target proxy (_proxy_call_callable_with_mapping): two new tests using inline
_DeprecatedProxy instances (num_warns=-1) confirm new_key wins over remapped old_key
regardless of argument order at the call site (old_first and new_first).
- Positional old arg + new-name kwarg: depr_collision_old_new(5, new=6) confirms the fix
fires correctly when the old name arrives via positional resolution.
- args_extra priority: new depr_collision_old_new_extra_wins fixture (args_extra={"new": 99})
pins the documented precedence — args_extra merge runs last and always wins over
both explicit new-name kwargs and remapped old-name values.
---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
… fix (#198) --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
… llms, FAQPage New troubleshooting entry: "I passed both the old and new argument names at the same time — which value wins?" Explains the precedence hierarchy: args_extra > explicit new-name kwarg > remapped old-name value > source defaults, with runnable examples for each tier. Also adds the precedence rule as an Anti-Pattern bullet to llms.txt and llms-full.txt so AI agents generating migration code are aware it is not dict-insertion-order-dependent, and a matching FAQ entry to the FAQPage JSON-LD in docs/overrides/main.html. --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
When a caller passes both the deprecated old name and its replacement new name in the same call, the old value was silently discarded. Add a UserWarning identifying the ignored argument so callers can clean up the call site. - `_build_call_plan`: collect collision pairs before remap; emit `UserWarning: Both \`old\` (deprecated) and \`new\` were supplied to \`fn()\`; \`old\` is ignored.` per pair when stream is set - Add test `test_user_warning_emitted_when_both_old_and_new_supplied` - Update troubleshooting.md, llms.txt, FAQPage JSON-LD, functions.md guide - Extend AGENTS.md sync table: "Public API behavior change" now explicitly includes docs/troubleshooting.md + FAQPage to prevent future omissions --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Applies ≥3-char arg naming across all collision-story docs examples: - troubleshooting.md: args_mapping, code blocks, prose, classmethod section - guide/functions.md: ARGS_REMAP, score/score_predictions examples, admonition - llms.txt: anti-pattern note, thin-adapter snippet - overrides/main.html: FAQ collision answer, positional-only FAQ --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
Documentation currently claims the collision UserWarning applies to proxy surfaces and uses a non-standard # warns: annotation format that should be corrected to match implemented behavior and docs-test conventions.
Copilot's findings
- Files reviewed: 13/13 changed files
- Comments generated: 4
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Add collision UserWarning to `_apply_args_mapping` in proxy.py (stacklevel=4) so `deprecated_class()` now emits UserWarning when both old and new kwarg names are supplied, matching the existing @deprecated decorator path. - proxy.py: UserWarning in `_apply_args_mapping` when `explicit_new` non-empty - proxy.py: fix stale `args_extra` docstrings (args_extra wins, not caller) - CHANGELOG.md + docs/llms-full.txt: fix single-char `x`/`old_x` example names - CHANGELOG.md: remove false `deprecated_instance()` args_mapping claim - docs/troubleshooting.md: fix non-standard `# warns: FutureWarning + UserWarning` format - tests: add UserWarning proxy test, ARGS_REMAP UserWarning test, async collision test --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…_args_mapping Replace the separate dict-comprehension + warning-loop (both iterating the same condition) with a single `collision_pairs` list computed once, then reused for building `explicit_new` and emitting UserWarning. --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Both deprecation.py and proxy.py had identical collision-detection + UserWarning logic. Extract to utils._apply_args_mapping_collisions; callers pass stacklevel accounting for the added frame (4 in deprecation, 5 in proxy). --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Table padding wastes tokens for AI agents reading the file; a bullet list carries identical information with less whitespace. --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Before submitting
What does this PR do?
Before this fix the dict-comprehension in
_apply_args_mapping(proxy) and_build_call_plan(deprecation) used last-write-wins semantics, so the winner depended on Python dict iteration order —fn(old=5, new=6)worked butfn(new=6, old=5)silently produced the wrong value.result.update(explicit_new)so new-name always wins regardless of call-site argument orderPR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃