Skip to content

fix: forward calls to POSITIONAL_ONLY target params correctly#194

Merged
Borda merged 14 commits into
mainfrom
fix/positional_only_target
Jun 13, 2026
Merged

fix: forward calls to POSITIONAL_ONLY target params correctly#194
Borda merged 14 commits into
mainfrom
fix/positional_only_target

Conversation

@Borda

@Borda Borda commented Jun 12, 2026

Copy link
Copy Markdown
Owner
Before submitting
  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Previously @deprecated(target=fn) where fn declared any param as positional-only (def fn(x, /): ...) raised TypeError at call time because the dispatcher always used target_func(**resolved_kwargs).

  • Add _split_positional_only_kwargs() helper in deprecation.py that extracts positional-only values from resolved_kwargs in declaration order so they can be forwarded as positional args
  • Detect POSITIONAL_ONLY params in packing() at decoration time; emit UserWarning naming affected params and store in new DeprecationConfig.target_positional_only field
  • Apply split dispatch in both wrapped_fn and async_wrapped_fn
  • Add positional_only_target fixture and deprecated_positional_only_source wrapper; new TestPositionalOnlyTarget class (5 regression tests)
  • Update docs/troubleshooting.md entry from workaround to supported
  • Update docs/llms.txt Known Limitations bullet
  • Add CHANGELOG Fixed entry

PR 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 🙃

Previously `@deprecated(target=fn)` where `fn` declared any param as
positional-only (`def fn(x, /): ...`) raised `TypeError` at call time
because the dispatcher always used `target_func(**resolved_kwargs)`.

- Add `_split_positional_only_kwargs()` helper in `deprecation.py` that
  extracts positional-only values from `resolved_kwargs` in declaration
  order so they can be forwarded as positional args
- Detect POSITIONAL_ONLY params in `packing()` at decoration time; emit
  `UserWarning` naming affected params and store in new
  `DeprecationConfig.target_positional_only` field
- Apply split dispatch in both `wrapped_fn` and `async_wrapped_fn`
- Add `positional_only_target` fixture and `deprecated_positional_only_source`
  wrapper; new `TestPositionalOnlyTarget` class (5 regression tests)
- Update `docs/troubleshooting.md` entry from workaround to supported
- Update `docs/llms.txt` Known Limitations bullet
- Add CHANGELOG Fixed entry

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 12, 2026 08:15
@dosubot dosubot Bot added the bug / fix Something isn't working label Jun 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a call-forwarding bug in @deprecated(target=callable) when the target callable declares positional-only parameters (e.g. def fn(x, /): ...), which previously caused a runtime TypeError due to always dispatching via target_func(**resolved_kwargs).

Changes:

  • Add positional-only detection at decoration time and split dispatch at call time to forward positional-only values positionally.
  • Extend DeprecationConfig with target_positional_only metadata and add regression tests covering decoration warning + call forwarding.
  • Update troubleshooting/docs contract (llms.txt) and add a changelog “Fixed” entry.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/deprecate/deprecation.py Detect target POSITIONAL_ONLY params, warn at decoration time, and split call-time dispatch for sync/async wrappers.
src/deprecate/_types.py Add DeprecationConfig.target_positional_only field and document its meaning.
tests/collection_targets.py Add positional_only_target callable fixture with a positional-only parameter.
tests/collection_deprecate.py Add deprecated_positional_only_source wrapper fixture (suppresses decoration-time UserWarning during import).
tests/unittests/test_deprecation.py Add regression tests covering warning-at-decoration + successful call forwarding paths.
docs/troubleshooting.md Update the positional-only target troubleshooting entry from “crash/workaround” to “supported”.
docs/llms.txt Update the Known Limitations bullet to reflect support since v0.10.
CHANGELOG.md Document the fix in the UnReleased “Fixed” section.

Comment thread docs/troubleshooting.md Outdated
Comment thread docs/troubleshooting.md
@kilo-code-bot

kilo-code-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental Review (17b9708)

The incremental commit 17b9708 chore(lint): add C901/PLR0912 complexity checks introduces:

  • New ruff lint rules C90 (McCabe complexity) and PLR0912 (too many branches) in pyproject.toml
  • Corresponding # noqa: C901, PLR0912 suppressions on complex functions in audit.py, deprecation.py, docstring/inject.py, and proxy.py
  • Corrected docs/troubleshooting.md POSITIONAL_ONLY example using print() with <details><summary>Output:...</summary> block (resolves prior inline feedback on docs format)
Files Reviewed (6 files)
  • docs/troubleshooting.md — docs format correction, no issues
  • pyproject.toml — lint configuration update, no issues
  • src/deprecate/audit.py — complexity noqa suppressions, correct
  • src/deprecate/deprecation.py — complexity noqa suppressions, correct
  • src/deprecate/docstring/inject.py — complexity noqa suppressions, correct
  • src/deprecate/proxy.py — complexity noqa suppressions, correct

Reviewed by step-3.7-flash-20260528 · 168,401 tokens

Borda and others added 6 commits June 12, 2026 12:25
- docs/troubleshooting.md: second code block (thin-adapter pattern) lacked
  `from deprecate import deprecated` and the `new_fn` definition; phmdoctest
  generates isolated test functions per block so imports do not carry over —
  resolved `NameError: name 'deprecated' is not defined` in `test_code_1448`
- docs/guide/audit.md: hardcoded `DeprecationConfig` repr in expected-output
  block was missing the new `target_positional_only=frozenset()` field added
  in this PR — resolved `AssertionError` in `test_code_49_output_102`

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- docs/troubleshooting.md: add `# warns: FutureWarning` to three old_fn()
  call-site lines per AGENTS.md convention — callers now see warning is emitted
- docs/overrides/main.html: rewrite FAQPage JSON-LD answer for the POSITIONAL_ONLY
  question from "crash/workaround" to "supported since v0.10" with accurate
  description of detection-time UserWarning and split dispatch behaviour
- CHANGELOG.md: append `(#194)` PR link to the Fixed entry per project convention

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
When @deprecated(target=NewCls) is applied to an __init__ source, _normalize_target
remaps the target to NewCls.__init__ (unbound). inspect.signature(NewCls.__init__)
then reports `self` as POSITIONAL_ONLY (it sits left of `/`) causing the frozenset to
become frozenset({'self', 'x'}) and the UserWarning to say `['self', 'x']` with the
unhelpful advice to remove `/` from `self`.

Exclude `self` and `cls` from the frozenset comprehension — both are always implicitly
positional and neither can be passed as a kwarg to a bound or unbound method.

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…ragile test assertion

Performance: `_split_positional_only_kwargs` previously called `inspect.signature`
on every dispatch for any target with POSITIONAL_ONLY params — ~65× the cost of the
underlying call after num_warns quota exhausted. Fix: pre-compute `param_order` at
decoration time and store as `DeprecationConfig.target_positional_only_order`
(`repr=False` so existing audit repr tests are unaffected). Dispatcher now receives
a `tuple[str, ...]` and iterates directly — zero `inspect.signature` calls at
call time. Empty tuple when no POSITIONAL_ONLY params exist.

Test: remove fragile `.filename.endswith("test_deprecation.py")` assertion from
`test_decoration_emits_user_warning` — `pytest.warns` already validates category
and message. Merge three structurally identical call-shape tests into one
`@pytest.mark.parametrize` with `pytest.param(..., id=...)` per project convention.

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…m, args_mapping, and exhaustion paths

- Async dispatch: `async_positional_only_target` + `deprecated_async_positional_only_source`
  fixture and `test_async_dispatch_forwards_positional_correctly` — verifies `async_wrapped_fn`
  split-dispatch path works (previously untested, any regression would go undetected)
- Two-param ordering: `positional_only_two_params_target(a, b, /, c)` + two-params wrapper and
  `test_two_positional_only_params_forwarded_in_order` — catches ordering bugs in the
  `param_order` iteration that single-param tests cannot expose
- args_mapping interaction: `deprecated_positional_only_with_args_mapping_source` with
  `args_mapping={"old_x": "x"}` + `test_args_mapping_renames_before_positional_split` —
  verifies rename-before-split sequencing; swap would produce TypeError
- stream=None and num_warns exhaustion: two inline-fixture tests verify split dispatch still
  forwards correctly when all warning emission is suppressed or quota is exhausted

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
`silent_old_fn` and `quota_old_fn` in TestPositionalOnlyTarget had `-> int: ...`
bodies — mypy reports `Missing return statement [empty-body]` because `...` is
only valid as a stub body in Protocol/abstract contexts, not in concrete functions
with a non-None return type.

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread src/deprecate/deprecation.py
Comment thread tests/unittests/test_deprecation.py
…targets with POSITIONAL_ONLY params

- Extend _split_positional_only_kwargs to pop self/cls from kw_args into pos_args so
  unbound __init__ targets receive the instance in the first positional slot rather than
  as a keyword argument — resolves TypeError on constructor-forwarding path
- Add OldPositionalOnlyClass fixture (collection_deprecate.py) and regression test
  test_constructor_forwarding_positional_only_succeeds in TestPositionalOnlyTarget

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread src/deprecate/deprecation.py
Comment thread src/deprecate/deprecation.py Outdated
… gate fires

- `_target_positional_only` previously excluded `self`/`cls` from the stored frozenset.
  For a target where `self` is the *only* POSITIONAL_ONLY param (`def __init__(self, /): ...`),
  the set was empty — the split-dispatch gate never fired — `target_func(**{'self': instance})`
  raised `TypeError`.  Fix: include `self`/`cls` in the stored set; use `_warn_positional_only`
  (set minus `{"self", "cls"}`) for the `UserWarning` text so user-facing messages are unchanged.
- Update `DeprecationConfig.target_positional_only` docstring to document the change.
- Correct misleading comment in `packing()`: `_normalize_target()` remaps class→`__init__` at
  decoration time, not call-time; `stored_target` keeps the user-facing class for audit tools.
- Add `SelfOnlyPositionalOnlyTarget` fixture (`def __init__(self, /): ...`), `OldSelfOnlyClass`
  deprecated wrapper, and `test_self_only_positional_only_constructor_succeeds` regression test.

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Comment thread src/deprecate/deprecation.py
Comment thread tests/unittests/test_deprecation.py Outdated
Comment thread tests/unittests/test_deprecation.py Outdated
- `_split_positional_only_kwargs` now enumerates `param_order` and gates the `self`/`cls` special-case on `i == 0`; non-receiver parameters that happen to be named `self` or `cls` are no longer incorrectly extracted into positional args
- Move `silent_old_fn` inline wrapper to `collection_deprecate.py` as `deprecated_positional_only_stream_none` (module-level singleton — no quota state)
- Move `quota_old_fn` inline wrapper to `collection_deprecate.py` as `make_deprecated_positional_only_num_warns_one()` factory; factory pattern chosen because `num_warns=1` wrapper carries mutable `warn_count` state — singleton would exhaust quota after first test call and fail subsequent runs
- Update `test_deprecation.py` imports and test bodies to use the new fixtures

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread tests/unittests/test_deprecation.py
Comment thread docs/troubleshooting.md
Borda and others added 2 commits June 13, 2026 02:25
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- pyproject.toml: add `C90` (McCabe cyclomatic complexity) and `PLR0912` (branch-count proxy for cognitive complexity) to `lint.extend-select`; `mccabe.max-complexity = 10` already configured
- src/deprecate/{deprecation,audit,proxy,docstring/inject}.py: suppress pre-existing violations on intentionally-complex core machinery with `# noqa: C901` / `# noqa: C901, PLR0912` on def lines
- docs/troubleshooting.md: replace inline return-value comments (`— returns N`) with `print()` calls + `<details>` output block per docs convention

---
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@Borda

Borda commented Jun 13, 2026

Copy link
Copy Markdown
Owner Author

CodeFactor found multiple issues last seen at fe7c770:

Very Complex Method

@Borda Borda merged commit 1742a83 into main Jun 13, 2026
49 of 50 checks passed
@Borda Borda deleted the fix/positional_only_target branch June 13, 2026 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug / fix Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants