Move coverage fail_under check to combined coverage environment#2909
Move coverage fail_under check to combined coverage environment#2909
Conversation
📝 WalkthroughWalkthroughAdjusts many test-coverage pragmas and pytest/coverage flags, adds msgspec-generated expectations and tests, always attempts Pydantic model rebuild during schema loading, improves import removal cleanup, and propagates collection/union options into the GraphQL parser. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
|
📚 Docs Preview: https://pr-2909.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging #2909 will degrade performance by 16.84%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | test_perf_deep_nested |
5.1 s | 6 s | -15.24% |
| ❌ | WallTime | test_perf_duplicate_names |
839.6 ms | 1,002.1 ms | -16.21% |
| ❌ | WallTime | test_perf_multiple_files_input |
3.1 s | 3.8 s | -16.52% |
| ❌ | WallTime | test_perf_large_models_pydantic_v2 |
3.1 s | 3.7 s | -16.84% |
| ❌ | WallTime | test_perf_all_options_enabled |
5.7 s | 6.7 s | -14.56% |
| ❌ | WallTime | test_perf_graphql_style_pydantic_v2 |
695.7 ms | 820.7 ms | -15.23% |
| ❌ | WallTime | test_perf_complex_refs |
1.7 s | 2 s | -16.34% |
| ❌ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.2 s | 2.6 s | -16.69% |
| ❌ | WallTime | test_perf_openapi_large |
2.5 s | 2.9 s | -15.28% |
| ❌ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.6 s | 1.9 s | -14.84% |
| ❌ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.7 s | 2 s | -16.62% |
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. ↩
koxudaxi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/datamodel_code_generator/model/msgspec.py (2)
215-220: API parameter ignored but preserved for compatibility.The
use_union_operatorparameter is no longer used in the implementation - the function now always usesUNION_OPERATOR_DELIMITER(line 220) and passesuse_union_operator=Trueto helper functions (line 217). The parameter is kept for backward API compatibility.However, the static analysis correctly identifies that the
noqa: FBT001directive is unnecessary:🔎 Fix unused noqa directive
-def get_neither_required_nor_nullable_type(type_: str, use_union_operator: bool) -> str: # noqa: ARG001, FBT001 +def get_neither_required_nor_nullable_type(type_: str, use_union_operator: bool) -> str: # noqa: ARG001
224-226: API parameter ignored but preserved for compatibility.Similar to the function above,
use_union_operatoris ignored and the function always usesUNION_OPERATOR_DELIMITER.The
noqa: FBT001directive is unnecessary per static analysis:🔎 Fix unused noqa directive
-def _add_unset_type(type_: str, use_union_operator: bool) -> str: # noqa: ARG001, FBT001 +def _add_unset_type(type_: str, use_union_operator: bool) -> str: # noqa: ARG001action.yml (1)
103-103: Consider handling extra-args through the ARGS array for robustness.The current approach of directly interpolating
${{ inputs.extra-args }}works for typical use cases, but handling it through the ARGS array would be more robust for edge cases with complex quoting.🔎 Proposed refactor
if [ -n "${{ inputs.profile }}" ]; then ARGS+=(--profile "${{ inputs.profile }}") fi + + if [ -n "${{ inputs.extra-args }}" ]; then + ARGS+=(${{ inputs.extra-args }}) + fi - datamodel-codegen "${ARGS[@]}" ${{ inputs.extra-args }} + datamodel-codegen "${ARGS[@]}"Note: This still relies on GitHub Actions interpolation for word splitting of
extra-args, but centralizes all argument handling in one place.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/config-types.yamlaction.ymlpyproject.tomlsrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/model/msgspec.pytox.ini
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T08:25:19.839Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2890
File: tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py:14-15
Timestamp: 2026-01-02T08:25:19.839Z
Learning: The datamodel-code-generator currently generates RootModel subclasses with an explicit `root` field annotation (e.g., `class StringType(RootModel[str]): root: str`). This is existing behavior of the code generator and should not be flagged as an issue introduced by new changes.
Applied to files:
src/datamodel_code_generator/__main__.py
🧬 Code graph analysis (2)
src/datamodel_code_generator/__main__.py (1)
src/datamodel_code_generator/__init__.py (1)
Error(300-309)
src/datamodel_code_generator/model/msgspec.py (1)
src/datamodel_code_generator/types.py (1)
_remove_none_from_union(298-359)
🪛 Ruff (0.14.10)
src/datamodel_code_generator/model/msgspec.py
215-215: Unused noqa directive (non-enabled: FBT001)
Remove unused noqa directive
(RUF100)
224-224: Unused noqa directive (non-enabled: FBT001)
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). (13)
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
pyproject.toml (1)
250-250: Coverage threshold enforcement moved to combined coverage environment.Setting
fail_under = 0here is intentional - the coverage threshold is now enforced in the combined coverage environment in tox.ini (line 91) with--fail-under=100. This centralizes coverage checking after combining results from all test environments, which is a more accurate approach than checking coverage per environment.tox.ini (2)
43-43: Per-environment coverage check disabled.The
--cov-fail-under=0flag disables coverage threshold enforcement during individual test runs, deferring the check to the combined coverage environment. This aligns with the PR objective.
91-91: No changes needed — the 100% coverage threshold is already properly configured.The
--fail-under=100requirement is an intentional, active part of the CI pipeline (thecoverageenvironment that combines parallel test runs). The codebase already meets this threshold:
- 35+
pragma: no covermarkers are properly placed for platform-specific imports, error paths, and deprecated codepyproject.tomlexplicitly omits untestable paths:tests/data/*,tests/main/test_performance.py, and*/_types/*- The
--skip-coveredflag ensures only genuinely uncovered code fails the check- This environment is part of active CI, confirming the threshold is regularly met
.github/workflows/config-types.yaml (1)
23-33: Workflow refactored to use local action with profile-driven invocations.The workflow now uses a local action (
uses: ./) with differentprofileparameters instead of external actions and shell commands. This simplifies the workflow and aligns with the profile-based configuration approach defined in pyproject.toml.src/datamodel_code_generator/model/msgspec.py (1)
390-392: Coverage exclusions aligned with PR objectives.The
# pragma: no coverannotations mark code paths not exercised by tests, which aligns with the PR's coverage strategy of requiring 100% coverage on tested paths while explicitly marking untested branches.Also applies to: 394-395, 406-410, 421-422
src/datamodel_code_generator/__main__.py (1)
377-377: Coverage exclusion for untested error path.The
# pragma: no coverannotation marks this error path as intentionally excluded from coverage. This aligns with the PR's coverage strategy of achieving 100% coverage on tested code while explicitly marking untested edge cases.action.yml (2)
10-23: Good change enabling profile-based configuration.Making these inputs optional with empty defaults allows the action to work with profile-driven workflows where configuration is specified in
pyproject.toml. This aligns well with the workflow changes mentioned in the PR summary.
77-93: Correct conditional argument construction.The bash array handling is implemented correctly. The conditional checks ensure that only non-empty inputs are added to the command, which properly supports both explicit arguments and profile-based configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/datamodel_code_generator/reference.py (1)
99-99: Clean up unusednoqadirective.Static analysis (Ruff RUF100) reports that the
noqa: PLR0913directive is unused since PLR0913 (too-many-arguments) is not enabled. While thepragma: no coverannotation is appropriate, the unusednoqadirective should be removed for code cleanliness.🔎 Proposed fix
- def dict( # noqa: PLR0913 # pragma: no cover + def dict( # pragma: no cover self, *,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/model/msgspec.pysrc/datamodel_code_generator/prompt.pysrc/datamodel_code_generator/reference.pysrc/datamodel_code_generator/types.pytests/data/expected/main/openapi/msgspec_no_use_union_operator.pytests/main/openapi/test_main_openapi.py
💤 Files with no reviewable changes (1)
- src/datamodel_code_generator/format.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/datamodel_code_generator/model/msgspec.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-02T08:25:19.839Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2890
File: tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py:14-15
Timestamp: 2026-01-02T08:25:19.839Z
Learning: The datamodel-code-generator currently generates RootModel subclasses with an explicit `root` field annotation (e.g., `class StringType(RootModel[str]): root: str`). This is existing behavior of the code generator and should not be flagged as an issue introduced by new changes.
Applied to files:
src/datamodel_code_generator/types.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/reference.py
📚 Learning: 2025-12-25T09:22:22.481Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2799
File: src/datamodel_code_generator/model/pydantic/__init__.py:43-43
Timestamp: 2025-12-25T09:22:22.481Z
Learning: In datamodel-code-generator project, defensive `# noqa: PLC0415` directives should be kept on lazy imports (imports inside functions/methods) even when Ruff reports them as unused via RUF100, to prepare for potential future Ruff configuration changes that might enable the import-outside-top-level rule.
Applied to files:
src/datamodel_code_generator/__main__.py
📚 Learning: 2025-12-18T13:43:16.235Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2681
File: tests/cli_doc/test_cli_doc_coverage.py:82-82
Timestamp: 2025-12-18T13:43:16.235Z
Learning: In datamodel-code-generator project, Ruff preview mode is enabled via `lint.preview = true` in pyproject.toml. This enables preview rules like PLR6301 (no-self-use), so `noqa: PLR6301` directives are necessary and should not be removed even if RUF100 suggests they are unused.
Applied to files:
src/datamodel_code_generator/__main__.py
🧬 Code graph analysis (4)
src/datamodel_code_generator/__main__.py (1)
src/datamodel_code_generator/__init__.py (1)
Error(300-309)
tests/data/expected/main/openapi/msgspec_no_use_union_operator.py (1)
src/datamodel_code_generator/model/msgspec.py (2)
Struct(109-209)field(297-302)
src/datamodel_code_generator/reference.py (1)
src/datamodel_code_generator/parser/jsonschema.py (2)
root_id(756-758)root_id(761-763)
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(99-101)run_main_and_assert(245-409)
🪛 Ruff (0.14.10)
src/datamodel_code_generator/reference.py
99-99: Unused noqa directive (non-enabled: PLR0913)
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). (13)
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.11 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.14 on Ubuntu
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (8)
src/datamodel_code_generator/prompt.py (1)
69-70: LGTM! Appropriate coverage exclusion for edge case.The
pragma: no coverannotation is appropriate here, as this else branch handles the rare case where an option lacks a description. The code logic is correct, and this annotation simply excludes an unlikely edge case from coverage requirements.src/datamodel_code_generator/__main__.py (1)
377-377: LGTM! Appropriate coverage exclusions for edge case validations.The
pragma: no coverannotations are reasonable for these edge cases:
- Lines 377 & 452: Validation error paths for keyword-only with incompatible Python versions (duplicated for Pydantic v1/v2 compatibility)
- Line 720: Repository boundary heuristic when searching for pyproject.toml
These are defensive code paths that should rarely execute in normal usage, making them appropriate candidates for coverage exclusion.
Also applies to: 452-452, 720-720
src/datamodel_code_generator/reference.py (1)
770-770: LGTM! Appropriate coverage exclusion for edge case.The
pragma: no coverannotation is reasonable for this URL reference resolution edge case where the file part matches the root_id. This is a defensive check that should rarely execute in normal usage.src/datamodel_code_generator/types.py (1)
347-348: LGTM! Appropriate coverage exclusions for edge cases.The
pragma: no coverannotations are reasonable for these edge cases:
- Line 347: Recursive nested union prefix handling during None removal
- Lines 650-657: Rare configuration where
use_generic_container=Truebutuse_standard_collections=False- Lines 781-782: Default SET handling path for non-standard, non-generic containers
These are defensive code paths or rare configuration combinations that are appropriate candidates for coverage exclusion.
Also applies to: 650-657, 781-782
tests/main/openapi/test_main_openapi.py (1)
3522-3538: LGTM! Well-structured test for msgspec without union operator.This test appropriately adds coverage for the
--no-use-union-operatorflag with msgspec.Struct output. It complements the existingtest_main_openapi_msgspec_use_union_operator(line 3453) by testing the opposite behavior—explicitly disabling the union operator syntax even when targeting Python 3.10+.The test structure follows established patterns in the file, and the use of
MSGSPEC_LEGACY_BLACK_SKIPis consistent with other msgspec tests.tests/data/expected/main/openapi/msgspec_no_use_union_operator.py (3)
5-9: LGTM! Correct imports for Union syntax.The imports correctly include
Unionfromtyping, which is necessary for theUnion[X, Y]syntax demonstrated throughout this file. All msgspec-specific imports are also present.
12-47: LGTM! Proper Union syntax throughout Struct classes.The generated Structs correctly use
Union[X, Y]syntax instead of the union operator (X | Y), which aligns with the--no-use-union-operatorflag. The combination ofUnion,Annotated, andUnsetTypefollows msgspec best practices for optional fields with metadata.Examples:
- Line 15:
Union[str, UnsetType]with default value- Lines 32-35:
Union[Annotated[str, Meta(...)], UnsetType]with UNSET default
50-72: LGTM! TypeAlias declarations use Optional consistently.The TypeAlias declarations correctly use
Optional[X]instead of the union operator syntax (X | None), maintaining consistency with the--no-use-union-operatorflag throughout the generated code.
koxudaxi
force-pushed
the
feature/increase-coverage-threshold
branch
2 times, most recently
from
a125025 to
864b084
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/openapi/nullable.yamlis excluded by!tests/data/**/*.yamland included by none
📒 Files selected for processing (17)
docs/cli-reference/model-customization.mdsrc/datamodel_code_generator/config.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/model/msgspec.pysrc/datamodel_code_generator/model/pydantic/__init__.pysrc/datamodel_code_generator/model/pydantic_v2/__init__.pysrc/datamodel_code_generator/util.pysrc/datamodel_code_generator/validators.pytests/data/expected/main/openapi/msgspec_no_use_union_operator.pytests/data/expected/main/openapi/msgspec_nullable.pytests/data/expected/main/openapi/msgspec_use_union_operator.pytests/data/expected/main/openapi/nullable.pytests/data/expected/main/openapi/nullable_strict_nullable.pytests/data/expected/main/openapi/nullable_strict_nullable_use_union_operator.pytests/data/expected/main/openapi/typed_dict_nullable.pytests/data/expected/main/openapi/typed_dict_nullable_strict_nullable.pytests/data/expected/main/openapi/use_default_kwarg.py
✅ Files skipped from review due to trivial changes (2)
- docs/cli-reference/model-customization.md
- src/datamodel_code_generator/config.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/datamodel_code_generator/model/msgspec.py
- src/datamodel_code_generator/format.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-25T09:23:08.506Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2799
File: src/datamodel_code_generator/util.py:49-66
Timestamp: 2025-12-25T09:23:08.506Z
Learning: In datamodel-code-generator, the is_pydantic_v2() and is_pydantic_v2_11() functions in src/datamodel_code_generator/util.py intentionally use global variable caching (_is_v2, _is_v2_11) on top of lru_cache for performance optimization. This dual-layer caching eliminates function call overhead and cache lookup overhead for frequently-called version checks. The PLW0603 linter warnings should be suppressed with # noqa: PLW0603 as this is a deliberate design choice.
Applied to files:
src/datamodel_code_generator/model/pydantic_v2/__init__.pysrc/datamodel_code_generator/util.py
📚 Learning: 2026-01-02T08:25:19.839Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2890
File: tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py:14-15
Timestamp: 2026-01-02T08:25:19.839Z
Learning: The datamodel-code-generator currently generates RootModel subclasses with an explicit `root` field annotation (e.g., `class StringType(RootModel[str]): root: str`). This is existing behavior of the code generator and should not be flagged as an issue introduced by new changes.
Applied to files:
src/datamodel_code_generator/model/pydantic_v2/__init__.pysrc/datamodel_code_generator/util.py
📚 Learning: 2025-12-25T09:22:22.481Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2799
File: src/datamodel_code_generator/model/pydantic/__init__.py:43-43
Timestamp: 2025-12-25T09:22:22.481Z
Learning: In datamodel-code-generator project, defensive `# noqa: PLC0415` directives should be kept on lazy imports (imports inside functions/methods) even when Ruff reports them as unused via RUF100, to prepare for potential future Ruff configuration changes that might enable the import-outside-top-level rule.
Applied to files:
src/datamodel_code_generator/util.py
🧬 Code graph analysis (5)
tests/data/expected/main/openapi/use_default_kwarg.py (1)
tests/data/expected/parser/openapi/openapi_parser_parse_modular/bar.py (1)
Field(6-7)
tests/data/expected/main/openapi/nullable.py (1)
tests/data/expected/parser/openapi/openapi_parser_parse_modular/bar.py (1)
Field(6-7)
src/datamodel_code_generator/util.py (1)
src/datamodel_code_generator/reference.py (1)
_BaseModel(82-139)
tests/data/expected/main/openapi/nullable_strict_nullable_use_union_operator.py (1)
tests/data/expected/parser/openapi/openapi_parser_parse_modular/bar.py (1)
Field(6-7)
tests/data/expected/main/openapi/msgspec_no_use_union_operator.py (1)
src/datamodel_code_generator/model/type_alias.py (1)
TypeAlias(37-42)
🪛 Ruff (0.14.10)
src/datamodel_code_generator/util.py
168-168: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
187-187: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (17)
src/datamodel_code_generator/model/pydantic_v2/__init__.py (1)
49-55: LGTM! Appropriate coverage exclusion for version-specific fallback.The
pragma: no coverannotation correctly excludes the Pydantic v1 fallback path when tests run exclusively with v2.src/datamodel_code_generator/model/pydantic/__init__.py (1)
39-47: LGTM! Consistent coverage strategy.The coverage exclusion mirrors the approach in
pydantic_v2/__init__.py, appropriately excluding the v1 compatibility path.src/datamodel_code_generator/validators.py (1)
43-51: LGTM! Appropriate exclusion for v2-only feature.Since the validators feature is v2-only (as noted in the comment), excluding the v1 fallback path from coverage is reasonable.
src/datamodel_code_generator/util.py (5)
20-23: LGTM! Appropriate exclusion for environment-specific import fallback.The
pragma: no coveron the ImportError handler is reasonable, as testing both the stdlibtomllib(Python 3.11+) and the fallbacktomliin a single environment is impractical.
64-82: LGTM! Reasonable exclusions for edge cases and environment-specific paths.Both coverage exclusions are appropriate:
- Line 66: Deprecated YAML bool format warning is an edge case
- Line 81: CSafeLoader fallback is environment-dependent (C extension availability)
141-172: LGTM! Pydantic v1 compatibility path correctly excluded.The coverage exclusions on lines 168 and 170 appropriately exclude the v1
root_validatorpath when tests run with v2.Note: Ruff reports the
# noqa: PLC0415on line 168 as unused (RUF100), but based on learnings, these defensive noqa directives should be retained for lazy imports to prepare for potential future Ruff configuration changes.
175-191: LGTM! Consistent coverage strategy for field validators.The coverage exclusions on lines 187 and 189 mirror the approach in
model_validator, appropriately excluding v1 compatibility code.Note: The RUF100 warning on line 187 should be ignored per the project's policy of retaining defensive
# noqa: PLC0415directives on lazy imports.
212-310: LGTM! Comprehensive v1 compatibility wrapper coverage exclusions.All Pydantic v1 fallback paths are appropriately excluded from coverage (lines 224, 289, 296, 303, 310), consistent with the overall strategy when tests run exclusively with v2.
tests/data/expected/main/openapi/typed_dict_nullable.py (1)
68-70: LGTM!The new union-typed fields are correctly defined as
NotRequiredwith appropriate union types. The absence ofNonein the union types is consistent with the non-strict nullable behavior for TypedDict, where "nullable" translates to "optional key" rather than "value can be None".tests/data/expected/main/openapi/nullable_strict_nullable_use_union_operator.py (1)
82-88: LGTM!The new fields correctly use the union operator syntax (
|) and include properFielddefinitions with descriptions. The description "not nullable" onmaybeValuewhile the type isstr | int | Nonereflects accurate preservation of the OpenAPI schema's description—the generator correctly transfers source metadata even when it doesn't align with Python's nullability semantics.tests/data/expected/main/openapi/msgspec_no_use_union_operator.py (1)
1-95: LGTM!This new expected output file correctly demonstrates msgspec Struct generation without the union operator:
- Uses
Union[...]syntax consistently instead of|operator (e.g.,Union[str, UnsetType]vsstr | UnsetType)- Uses
Optional[...]for type aliases (line 50, 69)- Properly imports from
typingmodule (Union,Optional,Annotated,TypeAlias)- Field definitions with
Metadescriptions andUNSETdefaults are correctly structuredThe file structure mirrors
msgspec_use_union_operator.pywith only the union syntax differing, which validates the--no-use-union-operatorflag behavior.tests/data/expected/main/openapi/msgspec_use_union_operator.py (1)
82-93: LGTM!The new fields correctly use the union operator syntax with msgspec patterns:
- Parenthesized unions for complex annotated types (lines 82-88, 89-92)
UnsetTypefor optional fields withUNSETdefaultMeta(description=...)for field metadataThe structure is consistent with the rest of the file and mirrors
msgspec_no_use_union_operator.pywith|syntax instead ofUnion[].tests/data/expected/main/openapi/nullable.py (1)
82-88: LGTM!The new fields are correctly defined:
maybeValueandnullableUnionuseField()with descriptions andNonedefaultsimpleUnioncorrectly uses a simple default assignment since it has no description metadataConsistent with the other Pydantic expected output files.
tests/data/expected/main/openapi/typed_dict_nullable_strict_nullable.py (1)
68-70: LGTM!The strict nullable variant correctly differentiates from the non-strict version:
nullableUnionincludesNonein the union (str | int | None) since the schema explicitly marks it as nullablemaybeValueandsimpleUnionremain withoutNonesince they're not explicitly nullable in the schemaThis properly validates the strict nullable mode preserves explicit nullability annotations.
tests/data/expected/main/openapi/nullable_strict_nullable.py (1)
82-88: LGTM!The new fields are correctly added with union operator syntax,
Field()wrappers for description metadata, and appropriateNonedefaults. The implementation is consistent with other Pydantic expected output files in this PR.tests/data/expected/main/openapi/use_default_kwarg.py (1)
85-88: Patterns are consistent across versions. The three fields follow an intentional pattern:maybeValueandnullableUnioninclude descriptions (viaField()in Pydantic andAnnotated[..., Meta(...)]in msgspec), whilesimpleUnionomits the description in both versions. The alignment with msgspec's union type handling is already correct.tests/data/expected/main/openapi/msgspec_nullable.py (1)
82-93: Verify the semantic difference in nullable handling between Pydantic and msgspec versions.The semantic difference is confirmed and correctly identified: the msgspec version implements "not nullable" for
maybeValueusing(str | int) | UnsetType(noNone), whereas the Pydantic version usesstr | int | None, making it nullable despite the description saying "not nullable". The TypedDict version usesNotRequired[str | int]withoutNone, providing a third approach.This reflects intentional library-specific design patterns:
- msgspec: Uses
UnsetTypeto distinguish unset (missing) fields from present values—enabling fine-grained control over optional vs. nullable semantics- Pydantic: Uses
| Noneto represent both null values and default behavior, conflating nullable with optional- TypedDict: Uses
NotRequiredfor optional fields without addingNonefor nullabilityThe consistent pattern across multiple expected outputs (msgspec_nullable.py, msgspec_use_union_operator.py, msgspec_no_use_union_operator.py, nullable.py, typed_dict_nullable.py, and strict_nullable variants) confirms this is intentional based on the code generator's design.
koxudaxi
force-pushed
the
feature/increase-coverage-threshold
branch
2 times, most recently
from
08e8c05 to
7ef06c1
Compare
…ironment-specific code
koxudaxi
force-pushed
the
feature/increase-coverage-threshold
branch
from
da494f0 to
af2e700
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/parser/test_base.py (1)
492-503: Remove unused fixture instead of excluding from coverage.The
escape_mapfixture is not used anywhere in the test file. Thetest_character_escapingtest uses theescape_charactersimport directly. Consider removing this fixture entirely rather than marking it with# pragma: no cover.🔎 Suggested removal of unused fixture
-@pytest.fixture -def escape_map() -> dict[str, str]: # pragma: no cover - """Provide escape character mapping for tests.""" - return { - "\u0000": r"\x00", # Null byte - "'": r"\'", - "\b": r"\b", - "\f": r"\f", - "\n": r"\n", - "\r": r"\r", - "\t": r"\t", - "\\": r"\\", - } - -tests/model/test_base.py (2)
43-45: Inconsistency with AI summary detected.The AI summary states this method changed from a classmethod to an instance method, but the code still shows the
@classmethoddecorator on line 43 and uses theclsparameter. The method signature has not changed.Additionally, the static analysis tool correctly identifies that the
# noqa: D102directive is unused because the D102 check is not enabled in your linting configuration.The
# pragma: no coveraddition is appropriate for this stub method in a test helper class.🔎 Optional cleanup to remove unused noqa directive
@classmethod -def get_data_type(cls, types: Types, **kwargs: Any) -> DataType: # noqa: D102 # pragma: no cover +def get_data_type(cls, types: Types, **kwargs: Any) -> DataType: # pragma: no cover pass
56-58: Same inconsistency and unused directive as class B.Similar to class B, the AI summary incorrectly states this method changed from a classmethod to an instance method. The
@classmethoddecorator remains on line 56, and the method signature usescls, notself.The
# noqa: D102directive is also unused here.The
# pragma: no coveraddition is appropriate for this stub method.🔎 Optional cleanup to remove unused noqa directive
@classmethod -def get_data_type(cls, types: Types, **kwargs: Any) -> DataType: # noqa: D102 # pragma: no cover +def get_data_type(cls, types: Types, **kwargs: Any) -> DataType: # pragma: no cover pass
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/main/jsonschema/test_main_jsonschema.pytests/main/openapi/test_main_openapi.pytests/main/test_public_api_signature_baseline.pytests/model/test_base.pytests/parser/test_base.py
✅ Files skipped from review due to trivial changes (1)
- tests/main/test_public_api_signature_baseline.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/main/openapi/test_main_openapi.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/model/test_base.py
44-44: Unused noqa directive (non-enabled: D102)
Remove unused noqa directive
(RUF100)
57-57: Unused noqa directive (non-enabled: D102)
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). (12)
- GitHub Check: 3.10 on macOS
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (3)
tests/parser/test_base.py (2)
39-44: LGTM! Appropriate coverage exclusion for abstract method stubs.These abstract method implementations are required by the
Parserbase class but are never executed in tests. The# pragma: no coverannotations are correctly applied.
290-297: Remove or clarify the intention of# pragma: no coverannotations on test helper methods.The
__init__assignment ofself._data(line 293) and therender()method (lines 295-297) in class D are marked with# pragma: no cover, but class D is a test helper that exists solely to support test infrastructure. The pragmas are appropriate, but add a comment explaining that D is a minimal test helper class whose implementation details are not meant to be tested. Alternatively, if these methods will never be called during test execution (which appears to be the case), consider removing the methods entirely or documenting this intentional design.tests/main/jsonschema/test_main_jsonschema.py (1)
288-293: Excluding the skipped CLI-exit test from coverage looks goodAdding
# pragma: no coverto this always-skippedtest_main_without_argumentskeeps coverage accounting cleaner without changing behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2909 +/- ##
===========================================
+ Coverage 99.40% 100.00% +0.59%
===========================================
Files 95 93 -2
Lines 16905 16836 -69
Branches 1990 1950 -40
===========================================
+ Hits 16804 16836 +32
+ Misses 52 0 -52
+ Partials 49 0 -49
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/datamodel_code_generator/parser/jsonschema.py (1)
291-291: Remove unusednoqadirective.The
# noqa: PLR1702directive is not needed as the rule is not enabled. Clean this up to avoid confusion.🔎 Proposed fix
- if isinstance(value, list): # pragma: no branch # noqa: PLR1702 + if isinstance(value, list): # pragma: no branchsrc/datamodel_code_generator/reference.py (1)
99-99: Remove unusednoqadirective.The
# noqa: PLR0913directive is not needed as the rule is not enabled. Clean this up to avoid confusion.🔎 Proposed fix
- def dict( # noqa: PLR0913 # pragma: no cover + def dict( # pragma: no cover
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/model/base.pysrc/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/parser/jsonschema.pysrc/datamodel_code_generator/reference.pytests/conftest.pytests/main/test_public_api_signature_baseline.pytests/parser/test_base.py
✅ Files skipped from review due to trivial changes (1)
- tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/datamodel_code_generator/parser/base.py
- src/datamodel_code_generator/main.py
- tests/parser/test_base.py
- tests/main/test_public_api_signature_baseline.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T08:25:19.839Z
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2890
File: tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py:14-15
Timestamp: 2026-01-02T08:25:19.839Z
Learning: The datamodel-code-generator currently generates RootModel subclasses with an explicit `root` field annotation (e.g., `class StringType(RootModel[str]): root: str`). This is existing behavior of the code generator and should not be flagged as an issue introduced by new changes.
Applied to files:
src/datamodel_code_generator/model/base.pysrc/datamodel_code_generator/parser/jsonschema.pysrc/datamodel_code_generator/reference.py
🧬 Code graph analysis (1)
src/datamodel_code_generator/reference.py (1)
src/datamodel_code_generator/parser/jsonschema.py (2)
root_id(756-758)root_id(761-763)
🪛 Ruff (0.14.10)
src/datamodel_code_generator/parser/jsonschema.py
291-291: Unused noqa directive (non-enabled: PLR1702)
Remove unused noqa directive
(RUF100)
src/datamodel_code_generator/reference.py
99-99: Unused noqa directive (non-enabled: PLR0913)
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). (14)
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: py312-isort5 on Ubuntu
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
src/datamodel_code_generator/model/base.py (1)
376-376: LGTM! Coverage annotations are appropriate.Both
# pragma: no branchannotations are correctly placed:
- Line 376: The final
elifin the examples handling chain—the uncovered branch is not meaningful to test- Line 916: Cache invalidation check where the
elsebranch would be a no-opThese changes align with the PR's objective to adjust test coverage configuration without affecting runtime behavior.
Also applies to: 916-916
src/datamodel_code_generator/format.py (1)
107-119: Good practice: Deprecation warning for unused property.The addition of a deprecation warning for the
has_type_aliasproperty is appropriate and follows Python best practices. The warning correctly usesDeprecationWarningwithstacklevel=2to point to the caller.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/test_input_model.py (1)
1370-1391: CWD duplication test behavior is fine; consider a slightly stricter assertionThe new precondition that inserts
cwdintosys.pathwhen missing makestest_input_model_cwd_already_in_pathreliably exercise “already present” behavior and thefinal_count <= initial_count + 1guard is reasonable.If you want this test to assert that your implementation never adds a duplicate when
cwdis already present, you could tighten it tofinal_count == initial_countin that branch and keep a separate test for the “missing cwd” case.tests/main/conftest.py (1)
479-542: Output validation logic looks good; clean up unusednoqaon_import_packageThe updated
_validate_output_filesand_import_packagepaths correctly validate single files and packages (with timing and error recording) and are now properly covered.Ruff reports the
# noqa: PLR0912on Line 491 as unused; since PLR0912 isn’t enabled, you can safely drop that directive to keep lint noise down.Proposed cleanup for the unused
noqamarker-def _import_package(output_path: Path) -> None: # noqa: PLR0912 +def _import_package(output_path: Path) -> None:tests/conftest.py (3)
103-103: Remove unused noqa directives.The
PLR0912andPLR0914rules are not enabled in the project's Ruff configuration, making these noqa directives unnecessary. OnlyARG001is relevant here.🔎 Suggested fix
-def _validate_cli_doc_marker(node_id: str, kwargs: CliDocKwargs) -> list[str]: # noqa: ARG001, PLR0912, PLR0914 +def _validate_cli_doc_marker(node_id: str, kwargs: CliDocKwargs) -> list[str]: # noqa: ARG001
276-277: RedundantPath()conversion.
CLI_DOC_COLLECTION_OUTPUTis already aPathobject (defined on line 24), so wrapping it inPath()again is unnecessary.🔎 Suggested fix
- with Path(CLI_DOC_COLLECTION_OUTPUT).open("w", encoding="utf-8") as f: + with CLI_DOC_COLLECTION_OUTPUT.open("w", encoding="utf-8") as f:
472-474: Use public API or document the private method dependency.Line 474 accesses
_load_value(), a private API of theinline_snapshotlibrary. While the code currently works, this creates a fragility risk since private methods can change without notice. Either:
- Verify if
get_snapshot_value()or another public API can replace this- Add documentation explaining why
_load_value()is necessary and cannot be avoidedCode context
else: # we need to normalize the external_file object's content as well assert _normalize_line_endings(expected._load_value()) == normalized_content
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/cli_doc/test_cli_doc_coverage.pytests/cli_doc/test_cli_options_sync.pytests/conftest.pytests/main/conftest.pytests/main/openapi/test_main_openapi.pytests/main/test_public_api_signature_baseline.pytests/model/test_base.pytests/parser/test_base.pytests/test_input_model.py
💤 Files with no reviewable changes (1)
- tests/model/test_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/main/test_public_api_signature_baseline.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/main/openapi/test_main_openapi.py (2)
tests/main/conftest.py (2)
output_file(99-101)run_main_and_assert(245-409)tests/test_main_kr.py (1)
output_file(44-46)
tests/main/conftest.py (1)
tests/conftest.py (2)
assert_directory_content(563-593)validate_generated_code(690-717)
🪛 Ruff (0.14.10)
tests/conftest.py
103-103: Unused noqa directive (non-enabled: PLR0912, PLR0914)
Remove unused noqa directive
(RUF100)
tests/main/conftest.py
491-491: Unused noqa directive (non-enabled: PLR0912)
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). (13)
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: py312-isort5 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
tests/parser/test_base.py (1)
283-284: LGTM! Simplified test class is sufficient.The minimal
Dclass (now containing only a docstring) is adequate for its usage in tests. SinceDis used exclusively as a type parameter (data_model_type=D) and not directly instantiated or invoked, removing the methods is appropriate cleanup aligned with the coverage-focused objectives of this PR.tests/cli_doc/test_cli_options_sync.py (1)
32-118: CLI option sync tests now properly included in coverageIncluding
TestCLIOptionMetaSyncin coverage is a sensible tightening; behavior is unchanged and the tests remain clear and robust.tests/test_input_model.py (2)
30-49: Helper assertion guards are clearer and fully coveredSwitching
_assert_exit_code,_assert_stderr_contains, and_assert_file_existsto explicitifchecks withpytest.fail(and no coverage suppression) keeps the helpers simple and makes their failure paths visible to coverage without changing semantics.
1125-1132: Pydantic v1 runtime mock remains correct with covered branchThe
mock_hasattrlogic keyed offcall_countstill correctly simulates a Pydantic v1 runtime for the first two checks and defers to the originalhasattrafterward; removing coverage suppression here is safe and preserves the intended behavior.tests/main/conftest.py (2)
132-164: Guard and validation branches are explicit and coverage-friendlyThe various helpers (
_assert_exit_code,_validate_extra_args,run_main_with_args,run_main_and_assert) now use straightforwardifguards withpytest.failand no coverage suppression. This keeps error paths well-documented, preserves prior behavior, and makes failures easier to diagnose in tests.Also applies to: 216-243, 323-361, 367-407
423-461: Version/exec‑skip helpers handle invalid inputs robustly
_parse_target_versionand_should_skip_execnow degrade gracefully on malformed--target-python-versionvalues and maintain the intended logic around skipping compile/exec based on target/runtime and Pydantic model type. No functional regressions spotted.tests/main/openapi/test_main_openapi.py (2)
3522-3572: New msgspec tests nicely extend nullable/oneOf/const coverageThe three added tests for msgspec (
no_use_union_operator,oneof_with_null_no_use_union_operator, andinline_const) are consistent with the existing msgspec suite, reuserun_main_and_assertcorrectly, and exercise important combinations of--no-use-union-operator,oneOfwithnull, and inlineconst. Nothing problematic here.
3728-3787: Type‑alias Py3.11 tests now counted in coverageBringing the mutual/recursive type‑alias tests for Python 3.11 into coverage (Pydantic and msgspec variants) without changing their logic keeps the forward‑ref behavior well exercised across models and backends.
tests/cli_doc/test_cli_doc_coverage.py (1)
23-84: CLI doc coverage tracking remains correct and is now fully coveredThe
collected_optionsfixture and the two coverage/reporting test classes behave as before, but are now visible to coverage. Usingget_canonical_optionagainst_cli_doc_itemscontinues to be a solid way to enforce and inspect CLI doc coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/main/conftest.py (1)
491-491: Remove unusednoqadirective.Static analysis indicates that
PLR0912(too-many-branches) is not enabled in the linter configuration, making this directive ineffective.🔎 Proposed fix
-def _import_package(output_path: Path) -> None: # noqa: PLR0912 +def _import_package(output_path: Path) -> None:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pyproject.tomltests/conftest.pytests/main/conftest.pytests/main/openapi/test_main_openapi.pytests/test_input_model.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- tests/test_input_model.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/conftest.py (1)
scripts/build_cli_docs.py (1)
collect_cli_docs(922-933)
tests/main/openapi/test_main_openapi.py (2)
tests/main/conftest.py (2)
output_file(99-101)run_main_and_assert(245-409)tests/test_main_kr.py (1)
output_file(44-46)
🪛 Ruff (0.14.10)
tests/main/conftest.py
491-491: Unused noqa directive (non-enabled: PLR0912)
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). (11)
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (12)
tests/main/openapi/test_main_openapi.py (3)
3522-3538: LGTM! New msgspec union operator test is well-structured.The test correctly validates msgspec.Struct generation without the union operator, using explicit
Union[X, Y]syntax instead of theX | Yoperator. The test follows established patterns and includes the appropriate@MSGSPEC_LEGACY_BLACK_SKIPdecorator.
3541-3557: LGTM! Test complements existing oneOf coverage.This test appropriately validates msgspec.Struct generation for oneOf schemas containing null types without using the union operator syntax. It provides good coverage alongside the union operator variant at line 3505.
3560-3573: No action needed. The test is correctly without the@MSGSPEC_LEGACY_BLACK_SKIPdecorator because its expected output is too simple (a basic Struct with minimal type annotations) to trigger the msgspec.Struct formatting differences that the decorator is designed to skip on Python 3.12 with black < 24.0.0. Unlike the more complex msgspec tests that use Union types or multiple complex fields, the inline const case doesn't generate the formatting issues that would cause test failures without the skip.tests/main/conftest.py (3)
237-242: LGTM!The removal of pragma annotations here is consistent with the PR objective of consolidating coverage checks. The code logic remains unchanged.
395-406: LGTM!The expected file deduction logic is unchanged - the prefix stripping loop correctly exits after the first match.
496-533: LGTM!The added
# pragma: no coverannotations on defensive branches (spec validation, exception handling) are appropriate. These paths are difficult to trigger in normal test execution and represent edge-case safeguards.tests/conftest.py (6)
103-107: LGTM!Good documentation practice to explain why the function is excluded from coverage - it clarifies that this code path only executes during the cli-docs tox job.
217-255: LGTM!The pragma annotations on the
collect_cli_docsandvalidation_errorsbranches are appropriate - these paths only execute during the--collect-cli-docsmode which runs in a separate tox job.
366-426: LGTM!The docstring updates consistently explain that these formatting helper functions are only called during assertion failures, which clarifies the
# pragma: no coverexclusions.
472-476: LGTM!Using
from Noneto suppress the exception chain is appropriate here - it prevents the confusing "During handling of the above exception..." message when the FileNotFoundError is an expected condition that's being converted to a more meaningful AssertionError.
312-333: LGTM!The added
config: pytest.Configparameter is properly used to check the--collect-cli-docsoption. Thenoqa: ARG001comment correctly applies to the unusedexitstatusparameter, notconfig.
478-487: Verify whether_load_value()is the intended public API forExternalFileobjects.The type branching correctly handles both string and
ExternalFilereturn types fromexternal_file(). However,_load_value()is a private method (underscore prefix), and the intended public API for accessing content frominline-snapshot'sExternalFileshould be verified. Consider checking the inline-snapshot library documentation (v0.31.1+) to confirm this is the recommended approach or if a public alternative exists.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/main/conftest.py (1)
491-491: Remove unusednoqadirective.The
noqa: PLR0912directive suppresses a rule that is not enabled in your ruff configuration, making it unnecessary.🔎 Proposed fix
-def _import_package(output_path: Path) -> None: # noqa: PLR0912 +def _import_package(output_path: Path) -> None:tests/conftest.py (1)
350-352: Remove unusednoqadirective.The
# noqa: PERF203on line 350 is unnecessary because PERF203 is not enabled. Remove it while keeping the# pragma: no branch.🔎 Proposed fix
- except ValueError: # noqa: PERF203 # pragma: no branch + except ValueError: # pragma: no branch continue
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/conftest.pytests/main/conftest.pytests/main/test_public_api_signature_baseline.pytests/test_input_model.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_input_model.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/conftest.py (2)
scripts/build_cli_docs.py (1)
collect_cli_docs(922-933)src/datamodel_code_generator/__main__.py (1)
_normalize_line_endings(763-765)
tests/main/test_public_api_signature_baseline.py (2)
src/datamodel_code_generator/config.py (2)
GenerateConfig(66-207)ParserConfig(210-339)src/datamodel_code_generator/enums.py (2)
StrictTypes(233-240)UnionMode(212-216)
🪛 Ruff (0.14.10)
tests/main/conftest.py
491-491: Unused noqa directive (non-enabled: PLR0912)
Remove unused noqa directive
(RUF100)
tests/conftest.py
350-350: Unused noqa directive (non-enabled: PERF203)
Remove unused noqa directive
(RUF100)
tests/main/test_public_api_signature_baseline.py
426-426: Unused noqa directive (non-enabled: PLR0911)
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). (11)
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on macOS
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (15)
tests/main/test_public_api_signature_baseline.py (2)
348-348: Coverage pragmas are appropriate for this PR scope.The non-functional coverage annotations (
# pragma: no coverand# pragma: no branch) align with the PR objective to adjust coverage thresholds and do not affect runtime behavior.Also applies to: 426-426, 510-510, 657-657, 682-682, 708-708, 730-730, 736-736
426-426: Remove unusednoqadirective.The
# noqa: PLR0911is unused because thePLR0911rule ("Too many return statements") is not enabled in your project's Ruff configuration. Either enable the rule if desired or remove the directive.🔎 Proposed fix
-def _normalize_type(tp: Any) -> str: # noqa: PLR0911 # pragma: no cover +def _normalize_type(tp: Any) -> str: # pragma: no cover⛔ Skipped due to learnings
Learnt from: koxudaxi Repo: koxudaxi/datamodel-code-generator PR: 2799 File: src/datamodel_code_generator/model/pydantic/__init__.py:43-43 Timestamp: 2025-12-25T09:22:22.481Z Learning: In datamodel-code-generator project, defensive `# noqa: PLC0415` directives should be kept on lazy imports (imports inside functions/methods) even when Ruff reports them as unused via RUF100, to prepare for potential future Ruff configuration changes that might enable the import-outside-top-level rule.Learnt from: koxudaxi Repo: koxudaxi/datamodel-code-generator PR: 2681 File: tests/cli_doc/test_cli_doc_coverage.py:82-82 Timestamp: 2025-12-18T13:43:16.235Z Learning: In datamodel-code-generator project, Ruff preview mode is enabled via `lint.preview = true` in pyproject.toml. This enables preview rules like PLR6301 (no-self-use), so `noqa: PLR6301` directives are necessary and should not be removed even if RUF100 suggests they are unused.tests/main/conftest.py (3)
237-242: Pragma removal aligns with coverage improvements.The removal of the coverage pragma from the
if expected_stdout_path is not Nonecondition is consistent with the PR's objective to increase coverage thresholds. The logic correctly validates thatcapsysis provided when stdout verification is requested.
395-406: LGTM! Expected filename derivation is correct.The pragma removal from line 395 aligns with the PR's coverage goals. The logic correctly derives the expected filename from the calling test function's name using frame introspection, stripping known test prefixes.
484-488: Pragma removal supports increased coverage threshold.The removal of coverage pragmas from the directory validation logic (line 484) and the exec check (line 487) is consistent with the PR's goal. The code correctly handles directory-based output validation and conditionally imports packages for runtime verification.
tests/conftest.py (10)
103-107: LGTM: Coverage pragma correctly applied.The docstring accurately explains that this validation only runs during CLI docs collection (which doesn't contribute to coverage), and the pragma placement is appropriate.
217-221: LGTM: Proper coverage granularity.Removing the pragma from the function signature while keeping it on specific branches (lines 235, 250) is the correct approach. The function is always called during collection, but certain branches only execute with
--collect-cli-docs.
235-239: LGTM: Branch pragma correctly placed.This branch only executes during CLI documentation collection, so the coverage pragma is appropriate.
250-255: LGTM: Error path pragma correctly placed.This block only executes when there are validation errors during CLI doc collection, so the coverage pragma is appropriate.
366-376: LGTM: Docstring accurately describes usage context.The updated docstring correctly explains that this function is only called during assertion failures, justifying the pragma.
379-383: LGTM: Docstring clarifies assertion-failure context.The enhanced docstring correctly explains when this formatting function is invoked, justifying the pragma placement.
403-407: LGTM: Docstring clarifies file-not-found context.The updated docstring correctly explains this function is only called when the expected file doesn't exist, justifying the pragma.
422-426: LGTM: Docstring clarifies diff context.The updated docstring correctly explains this function is only called when content doesn't match expectations, justifying the pragma.
478-487: Verify the type handling logic for external_file.The new
isinstance(expected, str)check suggests thatexternal_file()can return either a string or an object with a_load_value()method. The else branch (marked with pragma) handles the object case.Please verify:
- Under what conditions does
external_file()return a non-string object?- Is the
_load_value()method the correct way to extract content from that object?- Should the else branch have test coverage, or is it correctly marked as uncovered?
The logic appears sound, but confirming the type contract with the
inline_snapshotlibrary would be prudent.
543-547: LGTM: Branch pragma correctly placed on prefix-stripping loop.The
pragma: no branchis appropriate here because the loop is expected to break when it finds a matching prefix, preventing coverage from requiring the non-existent "complete without break" branch.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/main/test_public_api_signature_baseline.py (1)
426-462: Remove unusednoqadirective.The static analysis correctly identifies that
# noqa: PLR0911is unnecessary since PLR0911 (too-many-return-statements) is not enabled in the project's ruff configuration.Proposed fix
-def _normalize_type(tp: Any) -> str: # noqa: PLR0911 # pragma: no cover +def _normalize_type(tp: Any) -> str: # pragma: no cover
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/main/conftest.pytests/main/test_public_api_signature_baseline.pytests/test_input_model.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_input_model.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/main/conftest.py (1)
tests/conftest.py (3)
validate_generated_code(703-730)record_exec(299-302)record_error(304-306)
🪛 Ruff (0.14.10)
tests/main/test_public_api_signature_baseline.py
426-426: Unused noqa directive (non-enabled: PLR0911)
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). (13)
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.13 on Ubuntu
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
tests/main/conftest.py (5)
237-242: LGTM - Coverage pragma adjustments.The removal of
# pragma: no branchis appropriate since this branch is now being exercised by tests.
395-406: LGTM - Coverage pragma adjustments.The
# pragma: no branchremoval on line 395 aligns with the PR objective to consolidate coverage checks. The remaining pragma on line 401 for the prefix loop is appropriate.
484-488: LGTM - Pragma changed fromno covertono branch.The change from
# pragma: no coverto# pragma: no branchon theelifis semantically correct—the directory branch is now covered, but not all sub-branches may be taken in every test run.
536-540: Verify unconditionalsys.path.remove()is safe.The summary indicates this change removes a previous conditional check (
if str(parent_directory) in sys.path). The unconditionalremove()will raiseValueErrorif the path is not present.Reviewing the code flow:
sys.path.insert()at line 509 is called at the start of each loop iteration before any operations that could throw, so for each(parent_directory, _)inpackages, the path should always be present when the finally block runs. This appears safe, but please confirm no edge cases exist where the insert might be skipped.
491-534: LGTM - Coverage pragmas added to defensive branches.The added
# pragma: no covermarkers on:
- Line 496: The fallback packages discovery path
- Line 501: Empty packages early return
- Line 513, 526: Spec/loader None checks
- Line 533: Exception handler
These are appropriate for defensive code paths that are unlikely to be hit during normal test execution.
tests/main/test_public_api_signature_baseline.py (2)
348-362: LGTM - Coverage pragma added.Adding
# pragma: no coverto_type_to_stris reasonable since this helper may not have all code paths exercised in every test run.
717-725: LGTM - Pragma adjustments for pydantic v2 model_rebuild.The change from
# pragma: no coverto# pragma: no branchon line 717 is appropriate—hasattr(GenerateConfig, "model_rebuild")is always true under pydantic v2, so the branch is covered but the else path doesn't exist. The# pragma: no coveron theImportErrorhandler (line 723) correctly marks a defensive fallback that won't trigger in normal pydantic v2 environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/main/test_public_api_signature_baseline.py (1)
426-462: Remove unusednoqadirective.The
PLR0911rule is not enabled in your linter configuration, making this directive unnecessary. The# pragma: no coverannotation is appropriate.🔎 Proposed fix
-def _normalize_type(tp: Any) -> str: # noqa: PLR0911 # pragma: no cover +def _normalize_type(tp: Any) -> str: # pragma: no coverBased on static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/main/test_public_api_signature_baseline.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/main/test_public_api_signature_baseline.py (3)
src/datamodel_code_generator/config.py (1)
GenerateConfig(66-207)tests/data/expected/main/input_model/config_class.py (1)
GenerateConfig(117-248)src/datamodel_code_generator/enums.py (2)
StrictTypes(233-240)UnionMode(212-216)
🪛 Ruff (0.14.10)
tests/main/test_public_api_signature_baseline.py
426-426: Unused noqa directive (non-enabled: PLR0911)
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). (14)
- GitHub Check: py312-isort5 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.14 on macOS
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (6)
tests/main/test_public_api_signature_baseline.py (6)
348-362: LGTM on the pragma addition.The
# pragma: no coverannotation is appropriate for this test utility function.
510-514: LGTM on removing the early skip logic.Removing the conditional skip for
Parameter.emptyenables more thorough validation of default values between the baseline andGenerateConfig, improving test coverage.
656-661: LGTM on the stricter validation.Removing the conditional skip logic ensures all default values are validated for consistency between
GenerateConfigand thegenerate()signature.
672-681: LGTM on the enhanced validation.The test correctly preserves the callable default handling while removing the
Parameter.emptyskip, providing better validation coverage.
692-698: LGTM on removing the skip logic.This change aligns with the pattern of stricter validation across the test suite, ensuring complete default value consistency.
708-716: LGTM on the pragma annotations.The coverage pragmas are appropriately placed:
# pragma: no branchfor thehasattrcheck is correct since one branch is always taken based on Pydantic version# pragma: no coverfor theImportErrorhandler is reasonable as it's a fallback path
|
🎉 Released in 0.52.1 This PR is now available in the latest release. See the release notes for details. |
2 participants
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.