Add --field-type-collision-strategy option#2735
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 44 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 (16)
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 #2735 +/- ##
=======================================
Coverage 99.36% 99.36%
=======================================
Files 83 83
Lines 12068 12094 +26
Branches 1456 1458 +2
=======================================
+ Hits 11991 12017 +26
Misses 45 45
Partials 32 32
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:
|
CodSpeed Performance ReportMerging #2735 will not alter performanceComparing Summary
Footnotes
|
39d8c72 to
60a1ce5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/datamodel_code_generator/__main__.py (1)
24-40: Config wiring is correct; drop unused# noqa: UP045to satisfy RuffImporting
FieldTypeCollisionStrategy, adding it toConfig, and forwardingconfig.field_type_collision_strategyintogenerate()are all consistent with the rest of the configuration pipeline. The only problem is the new# noqa: UP045on the config field, which Ruff flags as an unusednoqa(rule not enabled).You can fix this by removing the
noqadirective:Proposed diff to remove unused
noqa- field_type_collision_strategy: Optional[FieldTypeCollisionStrategy] = None # noqa: UP045 + field_type_collision_strategy: Optional[FieldTypeCollisionStrategy] = NoneAlso applies to: 474-475, 781-783
src/datamodel_code_generator/parser/base.py (2)
859-859: Fix variable name typo.The variable is named
filed_namebut should befield_namefor consistency and clarity.🔎 Proposed fix
- filed_name = field.name + field_name = field.nameNote: This typo is also used on lines 867 and 877, which should be updated accordingly.
870-880: Add runtime type check before casting to DataModel.Line 876 casts
colliding_reference.sourcetoDataModelwithout verifying the actual type. Reference sources can beEnum,TypeAlias, or other model types. If the source is not aDataModel, line 879 (source.class_name = new_class_name) could fail or produce unexpected behavior.Additionally, line 877 uses the typo variable
filed_name(should befield_name).🔎 Proposed fix
if colliding_reference is not None: resolver = ModelResolver( exclude_names=all_class_names.copy(), snake_case_field=self.snake_case_field, remove_suffix_number=True, ) - source = cast("DataModel", colliding_reference.source) - resolver.exclude_names.add(cast("str", filed_name)) - new_class_name = resolver.add(["type"], cast("str", source.class_name)).name - source.class_name = new_class_name - all_class_names.add(new_class_name) + source = colliding_reference.source + if isinstance(source, DataModel): + resolver.exclude_names.add(cast("str", field_name)) + new_class_name = resolver.add(["type"], cast("str", source.class_name)).name + source.class_name = new_class_name + all_class_names.add(new_class_name)
🧹 Nitpick comments (1)
src/datamodel_code_generator/__init__.py (1)
273-282:FieldTypeCollisionStrategyenum design matches existing patterns; consider optional export.The enum (RenameField / RenameType with hyphenated values) is consistent with other strategy enums and the intended CLI surface. If you want it available via
from datamodel_code_generator import *, consider addingFieldTypeCollisionStrategyto__all__alongside the other public strategy enums; otherwise the current definition is fine.Also applies to: 884-899
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/cli_options.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/data/expected/main/jsonschema/field_has_same_name_rename_type.pytests/data/expected/main/jsonschema/field_type_collision_rename_type_double.pytests/data/jsonschema/field_type_collision_complex.jsontests/data/jsonschema/field_type_collision_rename_type_double.jsontests/main/jsonschema/test_main_jsonschema.py
🧰 Additional context used
🧬 Code graph analysis (9)
tests/data/expected/main/jsonschema/field_has_same_name_rename_type.py (1)
tests/data/expected/parser/openapi/openapi_parser_parse_modular/bar.py (1)
Field(6-7)
src/datamodel_code_generator/arguments.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(273-281)
src/datamodel_code_generator/parser/openapi.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(273-281)
src/datamodel_code_generator/parser/jsonschema.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(273-281)
src/datamodel_code_generator/parser/base.py (6)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(273-281)src/datamodel_code_generator/model/base.py (6)
class_name(679-681)class_name(684-688)base_class(668-670)field(333-335)name(658-660)all_data_types(706-710)src/datamodel_code_generator/model/dataclass.py (1)
field(141-146)src/datamodel_code_generator/model/pydantic/base_model.py (1)
field(90-104)src/datamodel_code_generator/reference.py (3)
Reference(141-200)reference(76-78)add(775-829)src/datamodel_code_generator/types.py (1)
all_data_types(424-430)
tests/main/jsonschema/test_main_jsonschema.py (2)
tests/main/conftest.py (2)
output_file(98-100)run_main_and_assert(244-408)tests/test_main_kr.py (1)
output_file(44-46)
tests/data/expected/main/jsonschema/field_type_collision_rename_type_double.py (1)
src/datamodel_code_generator/model/enum.py (1)
Enum(39-121)
src/datamodel_code_generator/parser/graphql.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(273-281)
src/datamodel_code_generator/__main__.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(273-281)
🪛 Ruff (0.14.8)
src/datamodel_code_generator/__main__.py
474-474: Unused noqa directive (non-enabled: UP045)
Remove unused noqa directive
(RUF100)
⏰ 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). (7)
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (15)
tests/data/expected/main/jsonschema/field_has_same_name_rename_type.py (1)
1-15: Expected model for rename‑type strategy looks consistentThe generated classes reflect the rename‑type behavior correctly: the field
TestObjectkeeps its original name while the referenced type is suffixed (TestObject_1). No issues spotted.tests/data/expected/main/jsonschema/field_type_collision_rename_type_double.py (1)
1-22: Rename‑type double‑collision expectations are coherentThe expected code cleanly disambiguates the enum and model names while keeping the
Order.Statusfield bound to the enum. This matches the described rename‑type semantics without obvious edge‑case issues.src/datamodel_code_generator/cli_options.py (1)
138-140: CLI docs metadata for--field-type-collision-strategyis wired correctlyThe option is registered with the correct canonical name and placed in the
FIELDcategory, staying in sync witharguments.pyand existing doc tooling.tests/data/jsonschema/field_type_collision_rename_type_double.json (1)
1-22: Schema covers the intended double‑collision scenarioThe JSON Schema cleanly models the
Statusenum plus an additionalStatus_1object, matching the expectations in the new golden file. No structural or validity problems seen.src/datamodel_code_generator/arguments.py (1)
24-24: New CLI argument for field/type collision strategy is correctly definedThe import and
field_options.add_argumentwiring look consistent with theFieldTypeCollisionStrategyenum and existing option patterns (group placement, choices, default handling, and help text). No behavioral issues identified here.Also applies to: 625-632
tests/data/jsonschema/field_type_collision_complex.json (1)
1-44: Complex collision schema is well‑formed and suitable for testsThe schema definition is valid and captures the layered enum/object relationships you need to stress the collision strategy logic. No changes needed.
tests/main/jsonschema/test_main_jsonschema.py (1)
3575-3633: New tests robustly cover the rename‑type collision strategyThe three added tests exercise:
- Basic rename‑type behavior on
field_has_same_name.json- The same scenario through the CLI‑docs harness
- A schema with an existing
_1suffix to validate idempotent renamingArguments, expected files, and marks (
cli_doc) are consistent with surrounding tests. No issues found.src/datamodel_code_generator/parser/jsonschema.py (1)
24-37: JsonSchemaParser wiring forfield_type_collision_strategyis clean and backward compatible.Importing
FieldTypeCollisionStrategy, adding it as a final keyword-only arg, and forwarding it tosuper().__init__all look correct and consistent with the parser API. Existing call sites remain unaffected because everything after*is keyword-only.Also applies to: 512-612, 613-709
src/datamodel_code_generator/parser/openapi.py (1)
20-35: OpenAPIParser correctly threadsfield_type_collision_strategythrough to the base parser.The added import, keyword-only parameter, and
super().__init__forwarding are consistent withJsonSchemaParserand keep the public constructor backward compatible while exposing the new collision strategy.Also applies to: 181-283, 285-381
src/datamodel_code_generator/parser/graphql.py (1)
16-28: GraphQLParser integration withFieldTypeCollisionStrategyis consistent and safe.The new enum import, added keyword-only
field_type_collision_strategyargument, and forwarding tosuper().__init__are aligned with the baseParserAPI and won’t break existing callers.Also applies to: 98-199, 200-297
src/datamodel_code_generator/__init__.py (1)
393-500:generate()correctly exposes and forwardsfield_type_collision_strategy.Adding
field_type_collision_strategyas a keyword-only parameter togenerate()and passing it through toparser_class(...)ensures the new collision strategy is configurable from the top-level API without impacting existing callers.Also applies to: 645-747
src/datamodel_code_generator/parser/base.py (4)
31-31: LGTM: Field type collision strategy configuration properly threaded through Parser.The import, parameter, and instance attribute correctly introduce the new collision strategy configuration to the parser.
Also applies to: 774-774, 931-931
850-853: LGTM: Collision strategy initialization is correct.The logic correctly determines whether to rename types based on the strategy and builds the set of existing class names for collision detection.
881-890: LGTM: Field renaming logic correctly implements RenameField strategy.The
elif not rename_typecondition ensures field renaming only occurs when NOT using theRenameTypestrategy. WhenRenameTypeis active, this block is skipped (either because a type was renamed or because there was no collision), which is the intended behavior.
860-869: The line numbers in this review comment are incorrect. The code snippet shown (containingallow_extra_fieldsandextra_fieldsconfiguration) is from lines 860-869, but the actual collision detection logic discussed in the comment is located at lines 1860-1880. The collision handling code at lines 1867-1868 does checkcolliding_reference is Noneto capture only the first collision, but this logic addresses field-to-type-name collisions withinfield_type_collision_strategy, which is a more specific scenario than multiple references with identical short names within a single field's data type. Correct the line number reference to lines 1860-1880 if the intent is to review this collision handling logic.Likely an incorrect or invalid review comment.
60a1ce5 to
c410609
Compare
Fixes: #2364
Summary by CodeRabbit
Release Notes
--field-type-collision-strategycommand-line option for handling field and type name collisions. Choose between RenameField (renames the field) or RenameType (renames the type) strategies.✏️ Tip: You can customize this high-level summary in your review settings.