Skip to content

fix: Misleading load order of kwargs vs. args - #289

Open
kstine93 wants to merge 6 commits into
laurentS:masterfrom
kstine93:master
Open

fix: Misleading load order of kwargs vs. args#289
kstine93 wants to merge 6 commits into
laurentS:masterfrom
kstine93:master

Conversation

@kstine93

@kstine93 kstine93 commented Aug 3, 2026

Copy link
Copy Markdown

Summary

Fix for issue #284 .

User raised an issue that args were indexed too soon - before kwargs were searched.
This was due to trying to make this operation a 1-liner, and resulted in Python attempting to parse args before checking kwargs.

User supplied this proposed fix, which looks good and appears to cause no other issues.

Checks done

  • tests pass
  • black formatter + linter pass

@Mukller Mukller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for fixing #284 — the two-step lookup correctly avoids the eager args[idx] evaluation that caused the IndexError, and it's applied consistently in both wrappers.

A few things before this can merge:

  1. Please drop the pyproject.toml version bump — releases are cut by the maintainer, and carrying 0.1.10 -> 0.1.11 in a bugfix PR will conflict with other pending changes.

  2. Prefer an explicit None check over truthiness: if not request: falls back to args[idx] whenever the kwarg value is falsy, which subtly changes semantics for an explicitly-passed request=None. if request is None: keeps the original contract and states the intent.

  3. The PR ships without a regression test. This exact crash class is why the issue exists; something like decorating a handler where request arrives as a keyword argument while positional args are shorter than idx, then asserting the endpoint responds instead of raising IndexError, would lock the fix in. The existing FastAPI extension tests in tests/test_fastapi_extension.py look like a good home for it.

@kstine93

Copy link
Copy Markdown
Author

Hey @Mukller ,

Thanks for the helpful PR comments -- all good points.
I've pushed some updates to resolve the issues you raised. Specifically:

  1. Please drop the pyproject.toml version bump -> resolved in aad4fb1
  2. Prefer an explicit None check over truthiness -> resolved in 7b35f2e
  3. The PR ships without a regression test. -> resolved in 3d1d634

Let me know what you think :)

SIDE NOTE:
for the test, I found the IndexError was only thrown when attempting to add class-based routes to the FastAPI app (which seems to be what the user did, given their use of self in their example code in issue #284).
I therefore needed to work outside the existing test structure a bit to get the IndexError to throw, as you'll see in the test I wrote. It's somewhat concerning to me that we have such different behavior when class-based routes are used -- do you think it's worthwhile to include more such tests in the future, where the app routes are set up in different ways?

@kstine93
kstine93 requested a review from Mukller August 25, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants