feat: add --allof-class-hierarchy option#2869
Conversation
... to control class hierarchy representation for allOf schemas fix pytest and pre-commit hooks for Windows environments Refs koxudaxi#2645
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (--allof-class-hierarchy)
participant Config as GenerateConfig / Config
participant Gen as generate() / runner
participant Parser as Parser / JSONSchemaParser
participant Merger as _merge_all_of_object
CLI->>Config: parse args -> set allof_class_hierarchy
Config->>Gen: run_generate_from_config(allof_class_hierarchy)
Gen->>Parser: instantiate(allof_class_hierarchy)
Parser->>Merger: parse schemas -> encounter allOf
alt mode == Always
Merger-->>Parser: return None (skip merge) %%[`#f6c`]%%
else mode == IfNoConflict
Merger-->>Parser: perform merge if no conflicts %%[`#cfe6ff`]%%
end
Parser-->>Gen: parsed models
Gen-->>CLI: emit generated code files
note right of Merger: Colored fragments mark new/changed control flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-25T09:22:22.481ZApplied to files:
🧬 Code graph analysis (1)src/datamodel_code_generator/_types/generate_config_dict.py (1)
⏰ 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)
🔇 Additional comments (1)
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 |
CodSpeed Performance ReportMerging #2869 will improve performance by 23.46%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_duplicate_names |
1,019.7 ms | 841.3 ms | +21.2% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.8 s | 3.1 s | +23.46% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.6 s | 2.2 s | +20.67% |
| ⚡ | WallTime | test_perf_openapi_large |
2.9 s | 2.4 s | +19.34% |
| ⚡ | WallTime | test_perf_complex_refs |
2 s | 1.7 s | +20.14% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
2 s | 1.6 s | +21.19% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
836.3 ms | 687.7 ms | +21.6% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
2 s | 1.6 s | +20.54% |
| ⚡ | WallTime | test_perf_all_options_enabled |
6.8 s | 5.6 s | +20.68% |
| ⚡ | WallTime | test_perf_deep_nested |
6.1 s | 5.1 s | +20.64% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.7 s | 3 s | +22.77% |
Footnotes
-
98 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.pre-commit-config.yaml (1)
36-36: Cross-platform Python interpreter selection implemented correctly.The logic properly handles both Unix and Windows environments by checking for the executable in the platform-specific directories. The fallback to tox environment setup ensures the hook works even on fresh clones.
Optional: Consider applying similar logic to the config-types hook.
For consistency, the hook at line 42 could benefit from the same cross-platform path selection, as it currently only uses the Unix path
.tox/dev/bin/python.src/datamodel_code_generator/__init__.py (1)
29-50: Consider exportingAllOfClassHierarchyvia__all__for API consistencyYou import
AllOfClassHierarchyat the top-level, but it isn’t listed in__all__alongsideAllOfMergeModeand other enums. Adding it would make the public surface more self-consistent for wildcard or documented exports.Minimal addition
from datamodel_code_generator.enums import ( @@ - "AllExportsCollisionStrategy", - "AllExportsScope", - "DateClassType", + "AllExportsCollisionStrategy", + "AllExportsScope", + "AllOfClassHierarchy", + "DateClassType",Also applies to: 1237-1259
tests/conftest.py (1)
454-475: External snapshot normalization fix is good; consider avoiding private API and improving failure outputNormalizing the
external_filecontent via_normalize_line_endings(expected._load_value())should resolve the CRLF/LF mismatch on Windows, but there are two trade-offs:
- It relies on the private
_load_value()method of the inline-snapshot object, which could break on library changes.- In this branch, assertion failures won’t show the rich diff + “how to fix snapshot” hint that the string path provides.
If inline-snapshot exposes a public way to get the loaded string (or if you can coerce it to a
strin a documented way), it would be safer to use that and then route both branches through the same diff/hint code path.docs/cli-reference/typing-customization.md (1)
5-8:--allof-class-hierarchyexample mixes two unrelated schemasThe description of
if-no-conflictvsalwaysis clear, but in the--allof-class-hierarchysection the Input Schema JSON (StringDatatype, ConstrainedStringDatatype, etc.) is the same schema later used for--allof-merge-mode, while the With Option / Without Option outputs show the playgroundEntity/Thing/Location/Personhierarchy.Those outputs can’t be derived from the shown input schema, which is likely to confuse readers. It would be better to:
- Either replace the Input Schema here with the actual
allof_class_hierarchy.jsonused to produce the Entity/Thing/Location/Person examples, or- Replace the outputs with the models generated from the currently shown schema.
Also applies to: 37-315
src/datamodel_code_generator/enums.py (1)
142-160: AllOfClassHierarchy enum is fine; consider adding it to__all__The new
AllOfClassHierarchyenum and its values ("if-no-conflict","always") look correct and align with the documented CLI behavior. For consistency withAllOfMergeModeand other enums, you may want to export it via__all__:Suggested `__all__` tweak
__all__ = [ @@ - "AllOfMergeMode", + "AllOfMergeMode", + "AllOfClassHierarchy",Also applies to: 230-252
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/jsonschema/allof_class_hierarchy.jsonis excluded by!tests/data/**/*.jsonand included by none
📒 Files selected for processing (18)
.pre-commit-config.yamldocs/cli-reference/index.mddocs/cli-reference/quick-reference.mddocs/cli-reference/typing-customization.mdsrc/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/cli_options.pysrc/datamodel_code_generator/enums.pysrc/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/parser/graphql.pysrc/datamodel_code_generator/parser/jsonschema.pysrc/datamodel_code_generator/parser/openapi.pytests/conftest.pytests/data/expected/main/jsonschema/allof_class_hierarchy.pytests/data/expected/main/jsonschema/allof_class_hierarchy_ref.pytests/main/jsonschema/test_main_jsonschema.pytests/main/test_public_api_signature_baseline.py
🧰 Additional context used
🧬 Code graph analysis (11)
src/datamodel_code_generator/__main__.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
tests/conftest.py (1)
src/datamodel_code_generator/__main__.py (1)
_normalize_line_endings(1396-1398)
tests/data/expected/main/jsonschema/allof_class_hierarchy_ref.py (1)
tests/data/expected/main/jsonschema/allof_class_hierarchy.py (5)
Person(34-37)Entity(10-12)Entity2(15-17)Thing(20-23)Location(26-31)
src/datamodel_code_generator/parser/graphql.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
src/datamodel_code_generator/arguments.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
src/datamodel_code_generator/parser/openapi.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
src/datamodel_code_generator/__init__.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
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(98-100)run_main_and_assert(244-408)
tests/main/test_public_api_signature_baseline.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
src/datamodel_code_generator/parser/jsonschema.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
tests/data/expected/main/jsonschema/allof_class_hierarchy.py (1)
tests/data/expected/main/jsonschema/allof_class_hierarchy_ref.py (5)
Entity(25-27)Entity2(30-32)Thing(35-38)Location(41-46)Person(12-22)
⏰ 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: 3.14 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.14 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (15)
tests/data/expected/main/jsonschema/allof_class_hierarchy_ref.py (4)
55-55: LGTM! AllOfClassHierarchy properly integrated.The new
allof_class_hierarchyparameter is correctly:
- Imported from the enum module
- Added to the Config model with a sensible default (
IfNoConflict)- Threaded through to the
generate()functionThe placement next to
allof_merge_modeis logical and maintains consistency.Also applies to: 518-518, 1577-1577
18-18: LGTM! GraphQL parser properly updated.The
allof_class_hierarchyparameter is correctly integrated into the GraphQL parser:
- Import added alongside related enums
- Parameter added to
__init__with consistent default- Properly forwarded to base
ParserclassThe changes align with the pattern used in other parsers.
Also applies to: 170-170, 287-287
20-20: LGTM! CLI option properly defined.The
--allof-class-hierarchyargument is well-implemented:
- Clear, descriptive help text explaining both modes
- Properly positioned in the typing_options group after related
--allof-merge-mode- Uses enum values for choices, ensuring type safety
- Default of
Noneallows Config defaults to applyAlso applies to: 583-590
12-23: Understand the conflict resolution strategy for thetypefield in merged allOf mode.The
Personclass merges fields fromThingandLocation, both defining atype: str | Nonefield with different defaults ('playground:Thing'vs'playground:Location'). The merged result usestype: Any | None = 'playground:Location'.This behavior is intentional: when
--allof-class-hierarchy if-no-conflict(default) detects conflicting field types from multiple schemas in allOf, it usesAnyas a type-safe fallback and applies the last schema's default value. To preserve type safety and class hierarchy, use--allof-class-hierarchy always, which generatesclass Person(Thing, Location)withtype: str | None = 'playground:Person'instead.src/datamodel_code_generator/parser/base.py (1)
29-29: LGTM! Clean integration of the new parameter.The
allof_class_hierarchyparameter is properly:
- Imported from the enum definition
- Added to the constructor with a sensible default (
IfNoConflict)- Stored as an instance attribute
- Positioned logically alongside the related
allof_merge_modeparameterThis maintains backward compatibility and follows existing code patterns.
Also applies to: 755-755, 878-878
tests/main/test_public_api_signature_baseline.py (1)
16-16: LGTM! Baseline signatures correctly updated.The test baseline signatures have been properly updated to reflect the new public API parameter:
- Import added to the enum imports section
- Parameter added to both
_baseline_generateand_BaselineParser.__init__with consistent defaults- Placement matches the actual implementation
This ensures the public API signature tests will continue to pass.
Also applies to: 126-126, 249-249
src/datamodel_code_generator/parser/openapi.py (1)
22-22: LGTM! Parameter properly threaded through inheritance.The
allof_class_hierarchyparameter is correctly:
- Imported from the core library
- Added to
OpenAPIParser.__init__with the same default as the base class- Forwarded to the superclass (
JsonSchemaParser) initializerThis completes the parameter propagation through the parser inheritance chain:
Parser→JsonSchemaParser→OpenAPIParser.Also applies to: 254-254, 371-371
src/datamodel_code_generator/parser/jsonschema.py (4)
26-43: LGTM!The
AllOfClassHierarchyimport is correctly placed within the existing import block fromdatamodel_code_generator.
701-703: LGTM!The new
allof_class_hierarchyparameter is well-placed alongside the relatedallof_merge_modeparameter, with theIfNoConflictdefault preserving backward-compatible behavior.
817-819: LGTM!The parameter is correctly forwarded to the base class initializer, following the established pattern.
2018-2028: LGTM!The early return when
allof_class_hierarchy == AllOfClassHierarchy.Alwayscorrectly bypasses the merge logic, causingparse_all_ofto preserve the class hierarchy through inheritance. The comment clearly documents the intent.tests/data/expected/main/jsonschema/allof_class_hierarchy.py (1)
10-37: AllOf hierarchy expected output looks consistentThe generated Entity/Thing/Location/Person hierarchy and field overrides look coherent for the new
--allof-class-hierarchybehavior; no issues spotted in this expected output.docs/cli-reference/index.md (1)
12-18: CLI index updates align with new optionThe Typing Customization count and the new
--allof-class-hierarchyentry (linking totyping-customization.md#allof-class-hierarchy) look consistent with the added docs and option.Also applies to: 31-32
docs/cli-reference/quick-reference.md (1)
27-57: Quick reference entries for--allof-class-hierarchylook correctBoth the Typing Customization table row and the Alphabetical Index entry for
--allof-class-hierarchymatch the new option’s intent and point to the correct section anchor.Also applies to: 198-205
src/datamodel_code_generator/cli_options.py (1)
164-201: CLI metadata for--allof-class-hierarchyis wired correctlyThe new
CLI_OPTION_METAentry for--allof-class-hierarchyis categorized under Typing, matching how the option is documented elsewhere; nothing problematic here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/datamodel_code_generator/parser/jsonschema.py (1)
2036-2038: Consider enhancing the comment to clarify MRO implications.The early-exit logic correctly implements the
Alwaysmode behavior by skipping allOf merging entirely. However, the method's docstring (lines 2029-2035) mentions that merging is sometimes necessary to "avoid MRO issues" when multiple$refs have conflicting property definitions.The current comment could be more explicit about this trade-off:
if self.allof_class_hierarchy == AllOfClassHierarchy.Always: # Skip merging to always preserve class hierarchies, even if multiple $refs # have conflicting properties. Users accept potential MRO issues in exchange # for explicit inheritance relationships. return NoneThis clarifies that the
Alwaysmode intentionally bypasses conflict detection to honor the user's preference for hierarchical representation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/parser/jsonschema.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/datamodel_code_generator/parser/jsonschema.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
src/datamodel_code_generator/__main__.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
🔇 Additional comments (6)
src/datamodel_code_generator/__main__.py (3)
55-55: LGTM! Import added correctly.The
AllOfClassHierarchyimport is properly positioned in alphabetical order and follows the existing import pattern.
518-518: LGTM! Good choice of default value.The new
allof_class_hierarchyfield is properly integrated into theConfigmodel with a sensible default (IfNoConflict) that ensures backward compatibility. The placement next toallof_merge_modeis logical given their related functionality.
1600-1600: LGTM! Parameter correctly wired through.The
allof_class_hierarchyconfiguration is properly passed through to thegenerate()function, completing the wiring from the Config model to the generation logic. The placement maintains logical grouping with the relatedallof_merge_modeparameter.src/datamodel_code_generator/parser/jsonschema.py (3)
28-28: LGTM: Import is correctly placed.The import of
AllOfClassHierarchyfollows the established pattern and is properly imported from the public API surface.
708-708: LGTM: Parameter with backward-compatible default.The new parameter is properly typed and defaults to
IfNoConflict, preserving existing behavior for users who don't specify this option.
824-824: LGTM: Parameter properly forwarded to parent class.The parameter is correctly passed to the base class constructor, enabling it to be stored and accessed as
self.allof_class_hierarchy.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2869 +/- ##
==========================================
+ Coverage 99.38% 99.40% +0.01%
==========================================
Files 91 91
Lines 15555 15591 +36
Branches 1827 1831 +4
==========================================
+ Hits 15460 15498 +38
+ Misses 50 49 -1
+ Partials 45 44 -1
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:
|
|
Thank you for the PR. |
makes sense - it was just the case that I didn't want to disable the pre-commit hook but this forced me to commit the pre-commit workflow fixes as well |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/datamodel_code_generator/_types/generate_config_dict.pysrc/datamodel_code_generator/_types/parser_config_dict.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/datamodel_code_generator/_types/parser_config_dict.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
src/datamodel_code_generator/_types/generate_config_dict.py (1)
src/datamodel_code_generator/enums.py (1)
AllOfClassHierarchy(155-159)
🪛 GitHub Actions: Check config types
src/datamodel_code_generator/_types/generate_config_dict.py
[error] 13-21: Config check failed: import order/membership change detected for AllOfClassHierarchy in datamodel_code_generator.enums. The check reports a mismatch in the expected vs actual import list (likely duplicate or reordering of AllOfClassHierarchy).
⏰ 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: py312-black23 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: py312-isort5 on Ubuntu
- GitHub Check: 3.12 on Ubuntu
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (2)
src/datamodel_code_generator/_types/generate_config_dict.py (1)
108-108: LGTM!The field is correctly typed and logically placed adjacent to the related
allof_merge_modeoption.src/datamodel_code_generator/_types/parser_config_dict.py (1)
16-16: LGTM!Both the import and field additions are correct:
- The import is in proper alphabetical order (before
AllOfMergeMode).- The field is appropriately typed and placed adjacent to the related
allof_merge_modeconfiguration.Also applies to: 95-95
OK, I jsut merged the PR for windows. I tested my windows 11 machine for WLS2 and Native Windows Python. |
|
@simontaurus |
done |
|
📢 Related PR Released: 0.51.0 PR #2871, which references this PR, has been released. See the release notes for details. |
|
🎉 Released in 0.51.0 This PR is now available in the latest release. See the release notes for details. |
2 participants
... to control class hierarchy representation for allOf schemas
+fix pytest and pre-commit hooks for Windows environments
Refs #2645
Background:
Merging restrictions from multiple superclasses into a single isolated class is not always the expected behaviour
See:
https://github.com/opensemanticworld/datamodel-code-generator/blob/a7230436298aa41d95ef5ef83f25b618eb0cbc5c/tests/data/jsonschema/allof_class_hierarchy.json
generating
https://github.com/opensemanticworld/datamodel-code-generator/blob/a7230436298aa41d95ef5ef83f25b618eb0cbc5c/tests/data/expected/main/jsonschema/allof_class_hierarchy.py
with the new option enabled vs
https://github.com/opensemanticworld/datamodel-code-generator/blob/a7230436298aa41d95ef5ef83f25b618eb0cbc5c/tests/data/expected/main/jsonschema/allof_class_hierarchy_ref.py
with the default setting
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.