Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 19 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughDocumentation examples were expanded to show per-target-model outputs (Pydantic v1/v2, dataclasses, TypedDict, msgspec) and help text was clarified. Tests were refactored to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2675 +/- ##
==========================================
- Coverage 99.52% 99.52% -0.01%
==========================================
Files 79 79
Lines 11178 11172 -6
Branches 1348 1348
==========================================
- Hits 11125 11119 -6
Misses 32 32
Partials 21 21
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:
|
![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 (2)
docs/cli-reference/model-customization.md (1)
18-29: Broaden--output-model-typesummary beyond GraphQL-only wordingThe table entry now says “Generate data models from GraphQL schema definitions,” but
--output-model-typeapplies to all supported inputs (JSON Schema, OpenAPI, GraphQL, etc.). Consider rephrasing to something like “Select the backend for generated data models (e.g., Pydantic v1/v2, dataclasses, TypedDict, msgspec)” so the summary matches the actual CLI behavior; the GraphQL focus can stay in the detailed section. The--use-frozen-fielddescription is clear and accurate.tests/main/jsonschema/test_main_jsonschema.py (1)
1963-1971: Path prefix inconsistency in model_outputs.Lines 1968-1970 use
"main/jsonschema/"prefix while lines 2016-2018 use"jsonschema/"prefix. While this may match actual file locations, consider standardizing the path structure for consistency across test expectations.Also applies to: 2015-2018
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/cli-reference/field-customization.md(2 hunks)docs/cli-reference/model-customization.md(6 hunks)docs/cli-reference/quick-reference.md(4 hunks)tests/main/graphql/test_main_graphql.py(1 hunks)tests/main/jsonschema/test_main_jsonschema.py(6 hunks)tests/main/openapi/test_main_openapi.py(1 hunks)tests/test_main_kr.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/main/openapi/test_main_openapi.py (2)
tests/test_main_kr.py (1)
output_file(44-46)tests/main/conftest.py (2)
output_file(94-96)run_main_and_assert(196-352)
tests/main/jsonschema/test_main_jsonschema.py (2)
tests/test_main_kr.py (1)
output_file(44-46)tests/main/conftest.py (2)
output_file(94-96)run_main_and_assert(196-352)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 3.9 on Windows
- GitHub Check: benchmarks
🔇 Additional comments (10)
tests/main/graphql/test_main_graphql.py (1)
30-38: Nice cli_doc metadata and docstring alignmentThe cli_doc marker (options, input_schema, cli_args, model_outputs) lines up cleanly with the parametrized outputs, and the updated docstring clearly explains
--output-model-typein this GraphQL context. No issues from my side.Also applies to: 44-48
tests/test_main_kr.py (1)
281-287: Well-structured coverage for--use-field-descriptionThe new cli_doc marker, golden output wiring, and explanatory docstring are consistent with the existing schema/inline description tests and exercise the option cleanly.
Also applies to: 289-295
tests/main/openapi/test_main_openapi.py (1)
2864-2896: Parametrized discriminator default tests and cli_doc look consistentThe new parametrized test plus cli_doc metadata correctly exercise
--use-one-literal-as-defaultfor both Pydantic v2 and dataclass outputs, with expected paths built viaEXPECTED_OPENAPI_PATH / expected_file. The dedicated 3.10/3.9 dataclass tests add clear coverage of version-specific behavior. No changes requested.docs/cli-reference/field-customization.md (2)
1342-1393: Pydantic v1/v2 split for--field-extra-keyslooks consistent and clearThe new examples correctly distinguish v1 (extra keys as Field kwargs) from v2 (extra keys in
json_schema_extrawithexamples), aligning with the described behavior for extra field metadata while keeping the schema-driven values visible.
1455-1523:--field-extra-keys-without-x-prefixmulti-model output is well-alignedThe v1 example shows sanitized keyword arguments (e.g.,
field_comment,register_,schema_,x_abc) while the v2 example moves the raw extension keys intojson_schema_extraand mapsx-reprtorepr, which matches the intent of the option and keeps invalid identifiers out of the signature.docs/cli-reference/model-customization.md (2)
2950-3440: GraphQL-based--output-model-typeexamples are coherent for Pydantic v1 and dataclassesThe updated section clearly demonstrates how a single GraphQL schema can be rendered into Pydantic v1 models (with
Field(..., alias='__typename')) and into dataclasses (withtypename__defaults) while keeping scalar aliases (Boolean,ID,Int,String) consistent. Cross-references between Film/Person/Planet/Species/Starship/Vehicle line up correctly, and the forward-ref updates for Pydantic are in place for the circular references.
4726-4839:--use-frozen-fielddescription and multi-backend outputs accurately capture behaviorThe new bullets correctly describe:
- Pydantic v1 using
Field(allow_mutation=False)plusvalidate_assignment=True,- Pydantic v2 using
Field(..., frozen=True)andAwareDatetimefor date-time,- Dataclasses ignoring this flag (no frozen fields).
The corresponding examples match that behavior and clearly show that only
readOnlyproperties (id,created_at) become frozen whilepassword(writeOnly) stays mutable.tests/main/jsonschema/test_main_jsonschema.py (3)
435-439: LGTM! Multi-model output support added correctly.The transition from
golden_outputtomodel_outputsenables the documentation to display outputs for both Pydantic v1 and v2 in tabbed format, while the test continues to verify both variants through parameterization.
4099-4139: Excellent parameterization and documentation.The test consolidation is well-executed with clear parameterization across three model types. The docstring (lines 4122-4126) effectively documents the behavioral differences for the
schemafield across TypedDict, dataclass, and Pydantic models, making this an excellent example test.
4198-4233: Well-structured parameterization with comprehensive documentation.The parameterization successfully consolidates tests for frozen field behavior across three model types. The docstring (lines 4221-4225) provides valuable documentation about the different implementations, noting that dataclasses silently ignore the flag—important information for users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/main/jsonschema/test_main_jsonschema.py (2)
4103-4143: Consider clarifying the CLI args in the documentation marker.The
cli_argsin the@pytest.mark.cli_docdecorator on line 4114 shows["--target-python-version", "3.11"], but the actual test execution on lines 4137-4142 includes["--output-model-type", output_model, "--target-python-version", "3.11"].Since this test demonstrates behavior across different model types (TypedDict, dataclass, Pydantic v2), consider either:
- Adding a representative
--output-model-typeto thecli_args(e.g., for pydantic_v2)- Or documenting in a comment that the CLI args vary per model type
This helps ensure the documentation accurately reflects the command needed to reproduce the documented output.
4202-4237: Similar CLI args documentation issue as reserved_field_name test.The
cli_argson line 4213 shows["--use-frozen-field"], but the actual test execution on line 4236 includes["--output-model-type", output_model, "--use-frozen-field"]. Since this test demonstrates different behavior across model types, consider adding a representative--output-model-typeto thecli_argsfor documentation clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/cli-reference/model-customization.md(6 hunks)docs/cli-reference/quick-reference.md(4 hunks)tests/main/graphql/test_main_graphql.py(1 hunks)tests/main/jsonschema/test_main_jsonschema.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/main/graphql/test_main_graphql.py
- docs/cli-reference/quick-reference.md
🧰 Additional context used
🧬 Code graph analysis (1)
tests/main/jsonschema/test_main_jsonschema.py (2)
tests/test_main_kr.py (1)
output_file(44-46)tests/main/conftest.py (2)
output_file(94-96)run_main_and_assert(196-352)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.9 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
docs/cli-reference/model-customization.md (3)
18-18: Table descriptions updated for clarity; minimal changes.Lines 18 and 29 clarify the supported model types in the table summaries. The changes are minor but improve consistency with the expanded documentation below.
Also applies to: 29-29
2948-3040: Multi-version output examples added to--output-model-typesection.The section now provides tabbed examples showing Pydantic v1 and v2 output differences. The examples correctly highlight version-specific behavior:
- Pydantic v1 (line 3013):
items: Optional[List[Any]]without explicit default- Pydantic v2 (line 3035):
items: Optional[List[Any]] = Nonewith explicit defaultThis demonstrates real API differences between versions and helps users understand version-specific output.
4324-4439: Multi-framework output examples added to--use-frozen-fieldsection.The section now includes tabbed examples for Pydantic v1, Pydantic v2, and dataclass outputs, demonstrating how
--use-frozen-fieldaffects each framework:
- Pydantic v1 (lines 4390-4391): Config with
validate_assignment = True; fields useallow_mutation=False- Pydantic v2 (lines 4413-4418): Uses
frozen=Truein Field definitions- Dataclass (lines 4433-4439): Fields remain unchanged (silently ignored per documentation at line 4331)
The examples are accurate and consistent with each framework's design. The documentation note at lines 4328-4331 correctly explains per-framework behavior.
tests/main/jsonschema/test_main_jsonschema.py (4)
435-456: LGTM! Good refactoring for multi-model documentation support.The addition of
model_outputsto the CLI doc marker enables multi-model tab display in documentation, and the updated docstring clearly explains the purpose of the test across different model types.
967-999: LGTM! Consistent multi-model documentation pattern.The
model_outputsaddition follows the same pattern as other parameterized tests and correctly maps to the existing test parameters.
2019-2046: LGTM! Consistent implementation.The multi-model outputs are properly mapped to the existing parameterization.
4250-4250: Good catch adding the missing flag!Adding
"--use-frozen-field"to the extra_args on line 4250 ensures this test actually validates the frozen field behavior when no readOnly fields are present.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/cli-reference/quick-reference.md(7 hunks)docs/cli-reference/typing-customization.md(4 hunks)tests/main/graphql/test_main_graphql.py(3 hunks)tests/main/jsonschema/test_main_jsonschema.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/main/jsonschema/test_main_jsonschema.py (1)
tests/main/conftest.py (2)
output_file(94-96)run_main_and_assert(196-352)
🪛 markdownlint-cli2 (0.18.1)
docs/cli-reference/quick-reference.md
30-30: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
30-30: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.9 on Windows
- GitHub Check: 3.9 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (9)
docs/cli-reference/quick-reference.md (1)
29-29: ✓ Addressed previous feedback on--output-model-typedescription.The updated description now correctly reflects that the option applies globally across input types (OpenAPI, JSON Schema, GraphQL), no longer misleadingly appearing GraphQL-specific. All four updated descriptions are clear and accurate:
--allof-merge-mode: Emphasizes constraint merging from root model references--enum-field-as-literal: Clarifies the conversion from Enum classes to Literal types--output-model-type: Now properly general: "Select the output model type (Pydantic v1/v2, dataclasses, TypedDict, msgspec)"--use-frozen-field: Specifies the readOnly property contextDescriptions are consistently updated in both the category table and alphabetical index.
Also applies to: 31-31, 86-86, 97-97, 158-158, 180-180, 207-207, 235-235
docs/cli-reference/typing-customization.md (3)
7-9: Enhanced option descriptions and comprehensive behavior documentation.The updated descriptions for
--allof-merge-modeand--enum-field-as-literalare now more precise:
--allof-merge-mode: Clarifies that it "merges only constraint properties (minLength, maximum, etc.) from parent schemas referenced in allOf," explicitly distinguishing constraint-only merging from full property inheritance.--enum-field-as-literal: More direct phrasing: "Convert all enum fields to Literal types instead of Enum classes."Also applies to: 28-33
45-256: Thorough input schema and comparative output examples.The extended test schema for
--allof-merge-modecovers critical edge cases:
- Root model constraints (ConstrainedStringDatatype, NonNegativeIntegerDatatype)
- Non-root-model allOf (ObjectWithAllOf, RefToArrayAllOf)
- Incompatible types and conflicting formats
- Nested allOf and constraints-only schemas
The "With Option" vs "Without Option" output comparison clearly demonstrates the semantic difference in constraint merging. This level of documentation is excellent for users understanding the feature's scope and limitations.
Also applies to: 260-592
682-789: Appropriate GraphQL schema for enum examples.Using GraphQL as the input format for
--enum-field-as-literalexamples is fitting, as GraphQL enums have a direct mapping to Literal types. The dual output comparison effectively shows the Literal-based vs Enum-based approaches.tests/main/jsonschema/test_main_jsonschema.py (4)
4111-4120: cli_args may need to include --output-model-type for accurate documentation.The
cli_argsarray (line 4114) doesn't specify--output-model-type, but the test is parameterized across three different output model types (TypedDict, dataclass, Pydantic v2). For the documentation to accurately reflect the commands for each model type, consider whethercli_argsshould be parameterized or if themodel_outputsapproach handles this automatically.Additionally, verify that the
model_outputskeys ("typeddict","dataclass","pydantic_v2") align with the documentation system's expected naming convention.
4210-4219: cli_args missing --output-model-type specification.Similar to the previous test, the
cli_args(line 4213) don't include--output-model-type, even though this test is parameterized across three model types. Themodel_outputsmapping may handle this, but it's worth verifying that documentation generation correctly shows the appropriate model type for each tab.
4269-4292: LGTM - Clear documentation of new allof-merge-mode feature.The addition of the
--allof-merge-modeoption with clear documentation is well implemented. The docstring effectively explains the behavior, and the use ofcomparison_output(line 4274) will help users understand the difference between merge modes.
435-439: No changes needed. The code is correct as-is.tests/main/graphql/test_main_graphql.py (1)
30-38: LGTM! Well-structured CLI documentation metadata.The
cli_docmarker correctly maps the parameterized test cases tomodel_outputsfor documentation purposes, and thecli_argsprovides a clear default example.
koxudaxi
deleted the
refactor/parameterize-e2e-tests-with-model-outputs
branch
1 participant
Summary
model_outputsto cli_doc markers for multi-model tab display in documentationSummary by CodeRabbit
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.