Expose plain arguments#454
Conversation
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1064 1105 +41
Branches 110 111 +1
=========================================
+ Hits 1064 1105 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
I'm interested in this! Is there an already existing solution? |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThe PR adds a new CLI flag Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Invocation
participant Config as GenerateConfig
participant Parser as OpenAPIParser
participant Generator as Code Generator
CLI->>Config: Pass --include-request-argument flag
Config->>Parser: Initialize with include_request_argument=true
Note over Parser: Flag stored in parser instance
Parser->>Parser: get_argument_list() called
alt include_request_argument enabled
Parser->>Parser: Inject Request argument if missing
Parser->>Parser: Add fastapi.Request import
end
Parser->>Generator: Provide operation with modified arguments
Generator->>Generator: Generate code with request: Request parameter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/cli-reference.md (1)
75-82: Documentation section added.The new
--include-request-argumentreference section is consistent with other entries in this auto-generated doc.Optional nit: the description could note that the
Requestargument is only injected when an operation does not already declare a parameter namedrequest(per the guard inparser.py). Since this file is regenerated fromcli_docmarkers intests/main/test_main.py, the wording would need to be updated at the marker'soption_description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli-reference.md` around lines 75 - 82, The new --include-request-argument doc text should state that the FastAPI Request argument is only injected when an operation does not already declare a parameter named "request" (this reflects the guard in parser.py), so update the option_description marker used to generate this section in tests/main/test_main.py to include that note; keep wording consistent with other auto-generated entries and mention the parser.py behavior as the reason for the conditional injection.tests/main/test_main.py (2)
265-311: Test for plain_arguments/plain_parameters template variables.The assertions exercise the new
plain_arguments/plain_parametersaccessors againstsnake_case_argumentsfor differentiation. Looks good.Optional: consider adding a multi-parameter case (e.g., a required path parameter plus an optional query parameter) so the comma-separated rendering and ordering of the new accessors is also covered. The current single-parameter spec doesn't exercise the join logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/test_main.py` around lines 265 - 311, Add a second scenario in test_custom_template_can_use_plain_arguments to exercise multi-parameter rendering: extend the OpenAPI spec (or create an additional test) to include two parameters (e.g., a required path parameter and an optional query parameter with a default) and regenerate using the same template_dir so the template variables operations[0].plain_arguments and plain_parameters are rendered as a comma-separated list; update assertions to expect the correct ordering and comma separation (e.g., "pet_id, limit" for plain_arguments and corresponding type annotations for plain_parameters) while keeping checks for snake_case_arguments unchanged to verify differentiation.
314-345: Verify the assertedrequest: Request = ...rendering.The assertion
"request: Request = ..." in generatedimplies a default of...in the rendered output, but the parser builds the argument asArgument(name='request', type_hint='Request', required=True)with no explicit default (perparser.py:454-461). If the template happens to render= ...for required arguments today, that's incidental — a stricter assertion would be"request: Request" in generated, which decouples the test from default-rendering behavior and won't break if the template changes.♻️ Proposed tightening
- assert "request: Request = ..." in generated + assert "request: Request" in generated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/test_main.py` around lines 314 - 345, The test test_include_request_argument currently asserts the exact string "request: Request = ..." but the parser constructs Argument(name='request', type_hint='Request', required=True) with no explicit default; update the assertion to look for the presence of the type hint only (e.g., assert "request: Request" in generated) so the test no longer depends on whether the template renders a default value; modify the assertion in test_include_request_argument accordingly to decouple it from default-rendering behavior.fastapi_code_generator/cli.py (1)
93-93: Consider addinghelp=text to the new Typer option.Most existing options in
main()also omithelp=, but this is a new flag whose behavior (auto-injecting aRequestparameter into operation signatures, only when one is not already declared) isn't obvious from the name alone. Adding a short help string would surface in--helpand the generated CLI snapshot:♻️ Suggested addition
- include_request_argument: bool = typer.Option(False, "--include-request-argument"), + include_request_argument: bool = typer.Option( + False, + "--include-request-argument", + help="Include a FastAPI Request argument in generated operation signatures.", + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastapi_code_generator/cli.py` at line 93, The new Typer option include_request_argument in main() (fastapi_code_generator/cli.py) lacks a help string; update its Typer.Option declaration to add a concise help= description that explains it will auto-inject a Request parameter into generated operation function signatures when one isn't already declared (e.g., "Auto-inject a FastAPI Request parameter into operations when not present"). Ensure the help text is short and appears in CLI --help and generated snapshots.fastapi_code_generator/parser.py (1)
198-211:plain_parametersdrops defaults — confirm this is the intended behavior.Unlike
argument/snakecase,plain_parametersemits onlyname: typewith no= default. For forwarding/declaration in user-defined interfaces this is fine, but if anyone wires this into adef {{ function_name }}({{ plain_parameters }})template directly, an optional argument will lose its default and a required-after-optional order can produce aSyntaxError. Worth a note in the docstring/template docs that this is for forwarding only.Also, the four
Operationproperties (arguments,snake_case_arguments,plain_arguments,plain_parameters) now all repeatOperation.merge_arguments_with_union(self.arguments_list). A tiny_merged_argumentscached property would centralize that and avoid recomputing the merge four times per operation in templates.♻️ Optional refactor
+ `@cached_property` + def _merged_arguments(self) -> List[Argument]: + return Operation.merge_arguments_with_union(self.arguments_list) + `@property` def arguments(self) -> str: # pragma: no cover - sorted_arguments = Operation.merge_arguments_with_union(self.arguments_list) - return ", ".join(argument.argument for argument in sorted_arguments) + return ", ".join(argument.argument for argument in self._merged_arguments)(apply the same substitution to
snake_case_arguments,plain_arguments,plain_parameters).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastapi_code_generator/parser.py` around lines 198 - 211, The plain_parameters property currently emits only "name: type" and drops default values, which can break direct insertion into function definitions and cause SyntaxError when optional arguments lose defaults; update plain_parameters (and document in its docstring or template docs) to include default expressions where present (e.g., "name: type = default") so optional/defaulted args are preserved, and add a cached helper property (e.g., _merged_arguments) that returns Operation.merge_arguments_with_union(self.arguments_list) and replace the repeated calls in snake_case_arguments, plain_arguments, and plain_parameters to use _merged_arguments to avoid recomputing the merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fastapi_code_generator/parser.py`:
- Around line 454-464: The synthetic request argument appended by
include_request_argument becomes a positional-defaulted parameter and gets
argument.default = UsefulStr('...') and an inconsistent import; fix by inserting
the synthetic Argument(name='request', type_hint='Request', required=True) at
the front of the arguments list (or tag it so the positional-default rewrite
skips it) instead of appending, and ensure you add
Import.from_full_path("fastapi.Request") to imports_for_fastapi; also normalize
other places that currently add "starlette.requests.Request" to use
"fastapi.Request" so all generated code imports Request from fastapi.
---
Nitpick comments:
In `@docs/cli-reference.md`:
- Around line 75-82: The new --include-request-argument doc text should state
that the FastAPI Request argument is only injected when an operation does not
already declare a parameter named "request" (this reflects the guard in
parser.py), so update the option_description marker used to generate this
section in tests/main/test_main.py to include that note; keep wording consistent
with other auto-generated entries and mention the parser.py behavior as the
reason for the conditional injection.
In `@fastapi_code_generator/cli.py`:
- Line 93: The new Typer option include_request_argument in main()
(fastapi_code_generator/cli.py) lacks a help string; update its Typer.Option
declaration to add a concise help= description that explains it will auto-inject
a Request parameter into generated operation function signatures when one isn't
already declared (e.g., "Auto-inject a FastAPI Request parameter into operations
when not present"). Ensure the help text is short and appears in CLI --help and
generated snapshots.
In `@fastapi_code_generator/parser.py`:
- Around line 198-211: The plain_parameters property currently emits only "name:
type" and drops default values, which can break direct insertion into function
definitions and cause SyntaxError when optional arguments lose defaults; update
plain_parameters (and document in its docstring or template docs) to include
default expressions where present (e.g., "name: type = default") so
optional/defaulted args are preserved, and add a cached helper property (e.g.,
_merged_arguments) that returns
Operation.merge_arguments_with_union(self.arguments_list) and replace the
repeated calls in snake_case_arguments, plain_arguments, and plain_parameters to
use _merged_arguments to avoid recomputing the merge.
In `@tests/main/test_main.py`:
- Around line 265-311: Add a second scenario in
test_custom_template_can_use_plain_arguments to exercise multi-parameter
rendering: extend the OpenAPI spec (or create an additional test) to include two
parameters (e.g., a required path parameter and an optional query parameter with
a default) and regenerate using the same template_dir so the template variables
operations[0].plain_arguments and plain_parameters are rendered as a
comma-separated list; update assertions to expect the correct ordering and comma
separation (e.g., "pet_id, limit" for plain_arguments and corresponding type
annotations for plain_parameters) while keeping checks for snake_case_arguments
unchanged to verify differentiation.
- Around line 314-345: The test test_include_request_argument currently asserts
the exact string "request: Request = ..." but the parser constructs
Argument(name='request', type_hint='Request', required=True) with no explicit
default; update the assertion to look for the presence of the type hint only
(e.g., assert "request: Request" in generated) so the test no longer depends on
whether the template renders a default value; modify the assertion in
test_include_request_argument accordingly to decouple it from default-rendering
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 896cc482-876a-4c15-86e0-e941af3ca58f
📒 Files selected for processing (10)
README.mddocs/cli-reference.mddocs/index.mddocs/llms-full.txtfastapi_code_generator/_types/generate_config_dict.pyfastapi_code_generator/cli.pyfastapi_code_generator/config.pyfastapi_code_generator/parser.pytests/main/test_main.pytests/test_config.py
github-actions
Bot
added
the
breaking-change-analyzed
Breaking Change AnalysisResult: No breaking changes detected Reasoning: The This analysis was performed by repository automation using PR labels and the |
4 participants
Sometimes the argument may need to forward to other user-defined interface, and the argument type will lead to syntax error.
Expose argument name only for such purpose.
Summary by CodeRabbit
New Features
--include-request-argumentCLI flag to include a FastAPIRequestargument in generated operation function signatures, enabling access to HTTP request context.Documentation
--include-request-argumentflag with usage examples.