-
-
Notifications
You must be signed in to change notification settings - Fork 426
Add --use-closed-typed-dict option to control PEP 728 TypedDict generation #2956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 6 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 ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR introduces a new CLI option Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Config as Config Layer
participant Parser as JSON Schema Parser
participant Generator as TypedDict Generator
CLI->>Config: Parse --use-closed-typed-dict flag
Config->>Parser: Initialize with use_closed_typed_dict
Parser->>Parser: Store flag in instance
Parser->>Generator: Process additionalProperties
alt use_closed_typed_dict = True
Generator->>Generator: Apply PEP 728 closed=True
else use_closed_typed_dict = False
Generator->>Generator: Skip PEP 728 conversion
end
Generator->>Generator: Generate standard TypedDict
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
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. 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 |
Generated by GitHub Actions
🤖 Generated by GitHub Actions
|
📚 Docs Preview: https://pr-2956.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/datamodel_code_generator/__main__.py (1)
124-127: Fixgenerate_cli_command()foruse_closed_typed_dict=falseby extendingBOOLEAN_OPTIONAL_OPTIONS.
Without this,datamodel-codegen --generate-cli-commandwill omit--no-use-closed-typed-dicteven when config sets it to false (and CLI default is True), producing the wrong command.Proposed fix
BOOLEAN_OPTIONAL_OPTIONS: frozenset[str] = frozenset({ "use_specialized_enum", "use_standard_collections", + "use_closed_typed_dict", })Also applies to: 569-570, 1011-1016
src/datamodel_code_generator/parser/jsonschema.py (1)
1303-1325: The early-return whenuse_closed_typed_dictis disabled breaks Pydantic model generation.The
set_additional_properties()early-return (lines 1313–1314) prevents writing anyadditionalProperties*keys toextra_template_datawhen the flag is disabled. However, Pydantic v1 and v2 models rely on theadditionalPropertieskey to configure theirconfig_extrabehavior—bothpydantic_v2/base_model.py:317andpydantic/base_model.py:391read this key to decide whether to allow extra fields. Whenuse_closed_typed_dict=False, Pydantic models no longer see this metadata and cannot respect schema constraints likeadditionalProperties: true.The early-return should only skip TypedDict-specific logic (e.g., setting
additionalPropertiesTypeanduse_typeddict_backport), but should still set the baseadditionalPropertiesboolean so other model types remain unaffected.
🤖 Fix all issues with AI agents
In @docs/cli-reference/typing-customization.md:
- Around line 1749-1801: The docs duplicate two sections for the flags
--use-closed-typed-dict and --no-use-closed-typed-dict with identical examples;
update both sections so they clearly state the default and show distinct
examples: in the --use-closed-typed-dict section state it is the default and
show a TypedDict declaration including closed=True (or PEP 728 extra_items
usage), and in the --no-use-closed-typed-dict section show the Compatible/mypy
example without closed=True (i.e., no closed parameter), and fix the usage
snippets to actually include the flag being documented (use the full command
including --use-closed-typed-dict or --no-use-closed-typed-dict). Ensure both
occurrences of these sections (the current block and the similar block around
lines 3276-3328) are updated the same way.
- Around line 1758-1764: Update the usage example for the datamodel-codegen
command to show the documented flag by adding --no-use-closed-typed-dict to the
example invocation so it reads datamodel-codegen --input schema.json
--output-model-type typing.TypedDict --no-use-closed-typed-dict; ensure the tip
block and the numbered note (1) still match the description of the
--no-use-closed-typed-dict option.
🧹 Nitpick comments (3)
src/datamodel_code_generator/arguments.py (1)
603-609: Consider clarifying default behavior in help text.The help text doesn't explicitly state whether PEP 728 TypedDict generation is enabled by default. While the phrasing "Use --no-use-closed-typed-dict for type checkers..." implies the positive flag is default, making this explicit would improve clarity.
♻️ Suggested enhancement
typing_options.add_argument( "--use-closed-typed-dict", - help="Generate TypedDict with PEP 728 closed=True/extra_items for additionalProperties constraints. " + help="Generate TypedDict with PEP 728 closed=True/extra_items for additionalProperties constraints (default: enabled). " "Use --no-use-closed-typed-dict for type checkers that don't yet support PEP 728 (e.g., mypy).", action=BooleanOptionalAction, default=None, )tests/main/jsonschema/test_main_jsonschema.py (2)
4262-4273: Make the CLI-doc example deterministic by including the same target/version and (optionally) explicit flag.Right now the
cli_docexample only passes--output-model-type typing.TypedDict, but the actual test/golden output appears to be tied to--target-python-version 3.10(and implicitly to the default for--use-closed-typed-dict). If defaults change, the doc snippet can drift from the asserted output.Proposed patch
@pytest.mark.cli_doc( options=["--use-closed-typed-dict", "--no-use-closed-typed-dict"], @@ input_schema="jsonschema/typed_dict_closed.json", - cli_args=["--output-model-type", "typing.TypedDict"], + cli_args=[ + "--output-model-type", + "typing.TypedDict", + "--target-python-version", + "3.10", + # Optional: make the doc example independent of the default + # "--use-closed-typed-dict", + ], golden_output="jsonschema/typed_dict_closed.py", )
4316-4336: Good coverage for disablingclosed=True, but consider also covering theextra_itemssuppression path.
test_main_typed_dict_no_closedvalidates theadditionalProperties: false -> closed=Truecase. Iftyped_dict_extra_items.jsonexercises theadditionalProperties: {type: ...} -> extra_items=...path, a sibling test (or parametrization) would ensure--no-use-closed-typed-dictdisables both PEP 728 outputs, not justclosed=True.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/llms-full.txtis excluded by none and included by none
📒 Files selected for processing (16)
docs/cli-reference/index.mddocs/cli-reference/quick-reference.mddocs/cli-reference/typing-customization.mdsrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/_types/generate_config_dict.pysrc/datamodel_code_generator/_types/parser_config_dicts.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/cli_options.pysrc/datamodel_code_generator/config.pysrc/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/parser/jsonschema.pysrc/datamodel_code_generator/prompt_data.pytests/data/expected/main/input_model/config_class.pytests/data/expected/main/jsonschema/typed_dict_no_closed.pytests/main/jsonschema/test_main_jsonschema.pytests/main/test_public_api_signature_baseline.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/data/expected/main/jsonschema/typed_dict_no_closed.py (1)
src/datamodel_code_generator/model/typed_dict.py (1)
TypedDict(50-177)
⏰ 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). (10)
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: py312-isort5 on Ubuntu
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (12)
tests/data/expected/main/jsonschema/typed_dict_no_closed.py (1)
1-14: Expected “no closed TypedDict” fixture looks correct. The generatedClosedModel(TypedDict)omitsclosed=True/extra_items, aligning with--no-use-closed-typed-dict.src/datamodel_code_generator/_types/parser_config_dicts.py (1)
111-112: Config surface addition is consistent (ParserConfigDict.use_closed_typed_dict).src/datamodel_code_generator/_types/generate_config_dict.py (1)
116-118: Config surface addition is consistent (GenerateConfigDict.use_closed_typed_dict).tests/main/test_public_api_signature_baseline.py (1)
136-138: Baseline signatures updated appropriately (new kwarg with defaultTrue).Also applies to: 268-270
tests/data/expected/main/input_model/config_class.py (1)
194-196: Expected generated config fixture updated correctly (newuse_closed_typed_dict).docs/cli-reference/quick-reference.md (1)
39-50: Documentation anchors are properly defined for both TypedDict flags.
The quick-reference updates correctly link to existing sections in typing-customization.md for both--no-use-closed-typed-dictand--use-closed-typed-dict, and the flags appear consistently across the alphabetical index and table.src/datamodel_code_generator/parser/base.py (1)
922-924: Parser wiring is correct: Flag properly stored onParserinstance and gated throughout the JSON Schema handler.The flag is wired from CLI →
__main__.pyinstantiation → stored atbase.py:922→ used injsonschema.py:1313to gate PEP 728 behavior (closed=True/extra_items=X) based onadditionalProperties. Downstream use inmodel/typed_dict.pyconfirms the pattern for generating closed TypedDicts when the flag is enabled.docs/cli-reference/index.md (1)
12-12: Keep doc counts/anchors in sync with generated CLI docs/tests.
The updated “29” count and new anchors look consistent; please ensure the referenced anchors exist indocs/cli-reference/typing-customization.mdand that the auto-generated docs tests still pass.Also applies to: 136-137, 190-190
src/datamodel_code_generator/cli_options.py (1)
206-207: CLI docs metadata additions look consistent.
No concerns here beyond ensuringtests/cli_doc/test_cli_options_sync.pyremains green.src/datamodel_code_generator/config.py (1)
152-152: Config surface addition looks good; please confirm end-to-end wiring.
Given this affects Python typing features (PEP 728), ensure parser/generator wiring and public API signature tests cover the new flag.Also applies to: 290-290
src/datamodel_code_generator/__main__.py (1)
1011-1016:use_closed_typed_dictis correctly propagated togenerate().The parameter is properly defined in
GenerateConfigDictand accepted by thegenerate()function through its**options: Unpack[GenerateConfigDict]signature.src/datamodel_code_generator/prompt_data.py (1)
81-81: Verify consistency with auto-generation process.The file header indicates this is auto-generated and should not be edited manually (lines 1-3), but this PR directly modifies it. Additionally, both
--no-use-closed-typed-dictand--use-closed-typed-dictshare identical descriptions, which doesn't help users understand the difference or default behavior.Consider regenerating this file using
python scripts/build_prompt_data.pyafter updating the source cli_doc markers, and differentiate the descriptions to indicate which is the default behavior (e.g., "--use-closed-typed-dict (default): Generate..." vs "--no-use-closed-typed-dict: Disable...").Also applies to: 116-116
Generated by GitHub Actions
🤖 Generated by GitHub Actions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2956 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 94 94
Lines 17726 17738 +12
Branches 2037 2038 +1
=========================================
+ Hits 17726 17738 +12
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:
|
Breaking Change AnalysisResult: No breaking changes detected Reasoning: PR #2956 adds the --use-closed-typed-dict/--no-use-closed-typed-dict CLI options to control PEP 728 TypedDict generation (closed=True/extra_items). The default value is True, which preserves the existing behavior that was introduced in PR #2922. This PR does not change the default behavior - it only adds a way to opt-out of PEP 728 TypedDict generation for users who need compatibility with type checkers that don't support PEP 728 (e.g., mypy). Since the default behavior remains unchanged (PEP 728 closed TypedDict is still generated by default), this is not a breaking change. It's a backward-compatible feature addition that gives users more control. This analysis was performed by Claude Code Action |
|
Hi @koxudaxi , thanks for resolving this issue this quickly. Just for my own knowledge when do you expect to release these changes? Thanks in advance. |
|
🎉 Released in 0.53.0 This PR is now available in the latest release. See the release notes for details. |
Fixes: #2951
Summary by CodeRabbit
Release Notes
New Features
--use-closed-typed-dictand--no-use-closed-typed-dictCLI options to control PEP 728 TypedDict closed/extra_items generation. Enabled by default for improved type checking; disable for compatibility with tools that don't yet support PEP 728.Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.