Avoid TYPE_CHECKING imports for Ruff modular output#3043
Merged
Conversation
Generated by GitHub Actions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CLI flags and config to control whether generated imports are placed inside TYPE_CHECKING, propagates that option through config, parser, generator, and CodeFormatter, updates Ruff command construction to conditionally preserve runtime imports, and updates docs, tests, and expected outputs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Parser as Parser
participant Formatter as CodeFormatter/Generator
participant Ruff as Ruff (subprocess)
participant FS as Filesystem
CLI->>Parser: provide args (use_type_checking_imports)
Parser->>Formatter: build formatter/config (resolve_use_type_checking_imports)
Formatter->>FS: write generated files
Formatter->>Ruff: run check/format (use _ruff_check_command, ruff_path, optional --unfixable)
Ruff-->>Formatter: return results
Formatter->>FS: finalize output (runtime imports preserved or TYPE_CHECKING used)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
|
📚 Docs Preview: https://pr-3043.datamodel-code-generator.pages.dev |
Merging this PR will not alter performance
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3043 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 85 86 +1
Lines 17883 18045 +162
Branches 2091 2098 +7
==========================================
+ Hits 17883 18045 +162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/cli-reference/template-customization.md (1)
2345-2351: State the default and paired flag here.Because this section is written under the negative form, it is hard to tell that
TYPE_CHECKINGimports stay enabled by default and that--use-type-checking-importsis the explicit opt-back-in form. A one-line note would make the CLI behavior much clearer.📝 Suggested wording
The `--no-use-type-checking-imports` flag prevents Ruff from moving generated model imports into `TYPE_CHECKING` blocks. This is useful for modular Pydantic output where referenced models need to be importable at runtime without calling `model_rebuild()` manually. +By default, TYPE_CHECKING imports remain enabled; `--use-type-checking-imports` is the +explicit form of that default behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli-reference/template-customization.md` around lines 2345 - 2351, Add a one-line clarification stating the default behavior and the paired flag: explain that by default generated model imports are wrapped in TYPE_CHECKING blocks (i.e., type-checking imports are enabled), and that --no-use-type-checking-imports disables that behavior while --use-type-checking-imports explicitly re-enables it; reference the flags --no-use-type-checking-imports, --use-type-checking-imports and the TYPE_CHECKING behavior in the same paragraph so readers immediately see the default and the opt-out/opt-back-in option.tests/test_format.py (1)
197-218: Cover the batch-formatting path too.
generate()applies Ruff to modular output viaCodeFormatter.format_directory()insrc/datamodel_code_generator/__init__.py:902-919, so this only locks down the stdin branch. A companion test forformat_directory(..., use_type_checking_imports=False)would protect the actual issue-3038 path.🧪 Suggested companion test
+def test_format_directory_ruff_check_without_type_checking_imports( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(tmp_path) + formatter = CodeFormatter( + PythonVersionMin, + formatters=[Formatter.RUFF_CHECK], + use_type_checking_imports=False, + ) + output_dir = tmp_path / "output" + output_dir.mkdir() + + with mock.patch("subprocess.run") as mock_run: + formatter.format_directory(output_dir) + + mock_run.assert_called_once_with( + ("ruff", "check", "--fix", "--unsafe-fixes", "--unfixable", "TC001,TC002,TC003", str(output_dir)), + capture_output=True, + check=False, + cwd=str(tmp_path), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_format.py` around lines 197 - 218, Add a companion test that exercises the batch-formatting path by invoking CodeFormatter.format_directory (the path exercised by generate()) with use_type_checking_imports=False; in the new test (similar to test_format_code_ruff_check_formatter_without_type_checking_imports) chdir to tmp_path, create a sample Python file under the directory, patch subprocess.run (mock.patch("subprocess.run")) to return a stdout, call formatter.format_directory(tmp_path) and assert formatted output or that subprocess.run was invoked with arguments matching the Ruff invocation for file input (file path(s) and cwd set to str(tmp_path)) to cover the modular/batch branch and protect the issue-3038 path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli-reference/quick-reference.md`:
- Line 155: Add the positive CLI flag `--use-type-checking-imports` alongside
the existing `--no-use-type-checking-imports` entry: update the Template
Customization table row to include a parallel entry for
`--use-type-checking-imports` (linking to
template-customization.md#use-type-checking-imports or the same anchor pattern
as the negative flag) and add the `--use-type-checking-imports` entry into the
alphabetical index on the same page (quick-reference.md) so both
BooleanOptionalAction flags are listed consistently; ensure the flag label and
description mirror the negative-flag wording but expressed positively (e.g.,
"Keep generated model imports available at runtime when using Ruff fixes.") and
match formatting of other flag entries.
In `@src/datamodel_code_generator/arguments.py`:
- Around line 421-427: The flag "--use-type-checking-imports" is registered on
model_options but belongs to the template/formatter group; move the add_argument
call so it registers on template_options instead of model_options. Locate the
existing model_options.add_argument(...) invocation for
"--use-type-checking-imports" and change it to
template_options.add_argument(...), preserving the help text,
action=BooleanOptionalAction and default=None so CLI behavior and docs remain
the same.
In `@src/datamodel_code_generator/format.py`:
- Around line 415-420: The _ruff_check_command currently assumes a "ruff" string
and callers like apply_ruff_lint and format_directory don't pass a resolved
path; change _ruff_check_command to resolve the executable internally by calling
self._find_ruff_path() as the default ruff path (use that value instead of the
literal "ruff"), then build and return the command tuple as before; update the
_ruff_check_command signature/implementation to call self._find_ruff_path() when
no explicit path provided so all callers (apply_ruff_lint, format_directory)
automatically get the correct Ruff executable.
---
Nitpick comments:
In `@docs/cli-reference/template-customization.md`:
- Around line 2345-2351: Add a one-line clarification stating the default
behavior and the paired flag: explain that by default generated model imports
are wrapped in TYPE_CHECKING blocks (i.e., type-checking imports are enabled),
and that --no-use-type-checking-imports disables that behavior while
--use-type-checking-imports explicitly re-enables it; reference the flags
--no-use-type-checking-imports, --use-type-checking-imports and the
TYPE_CHECKING behavior in the same paragraph so readers immediately see the
default and the opt-out/opt-back-in option.
In `@tests/test_format.py`:
- Around line 197-218: Add a companion test that exercises the batch-formatting
path by invoking CodeFormatter.format_directory (the path exercised by
generate()) with use_type_checking_imports=False; in the new test (similar to
test_format_code_ruff_check_formatter_without_type_checking_imports) chdir to
tmp_path, create a sample Python file under the directory, patch subprocess.run
(mock.patch("subprocess.run")) to return a stdout, call
formatter.format_directory(tmp_path) and assert formatted output or that
subprocess.run was invoked with arguments matching the Ruff invocation for file
input (file path(s) and cwd set to str(tmp_path)) to cover the modular/batch
branch and protect the issue-3038 path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5dbeb895-e565-4bd6-aeb1-c4cc97eb26e7
⛔ Files ignored due to path filters (1)
docs/llms-full.txtis excluded by none and included by none
📒 Files selected for processing (18)
docs/cli-reference/index.mddocs/cli-reference/quick-reference.mddocs/cli-reference/template-customization.mdsrc/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/_types/generate_config_dict.pysrc/datamodel_code_generator/_types/parser_config_dicts.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/cli_options.pysrc/datamodel_code_generator/config.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/prompt_data.pytests/data/expected/main/input_model/config_class.pytests/data/expected/main/openapi/no_use_type_checking_imports_internal.pytests/main/test_main_general.pytests/main/test_public_api_signature_baseline.pytests/test_format.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/datamodel_code_generator/format.py (1)
415-420:⚠️ Potential issue | 🟠 MajorResolve Ruff executable lookup inside
_ruff_check_command().
apply_ruff_lint()andformat_directory()call this helper withoutruff_path, so they still fall back to the literal"ruff"whileapply_ruff_check_and_format()uses_find_ruff_path(). In environments where Ruff is only installed next tosys.executable,Formatter.RUFF_CHECKand batchformat_directory()will fail even though the combined check+format path works.♻️ Proposed fix
- def _ruff_check_command(self, *paths: str, ruff_path: str = "ruff") -> tuple[str, ...]: + def _ruff_check_command(self, *paths: str, ruff_path: str | None = None) -> tuple[str, ...]: """Build the Ruff check command for the current formatter settings.""" + if ruff_path is None: + ruff_path = self._find_ruff_path() command: tuple[str, ...] = (ruff_path, "check", "--fix", "--unsafe-fixes") if not self.use_type_checking_imports: command += ("--unfixable", "TC001,TC002,TC003") return (*command, *paths)Verify that both callers still omit
ruff_pathwhile the helper defaults to the literal command. Expected result: the helper signature still showsruff_path: str = "ruff", and theapply_ruff_lint()/format_directory()call sites do not passruff_path=.#!/bin/bash set -euo pipefail sed -n '374,451p' src/datamodel_code_generator/format.py python - <<'PY' from pathlib import Path text = Path("src/datamodel_code_generator/format.py").read_text(encoding="utf-8") print("helper defaults to literal 'ruff':", 'def _ruff_check_command(self, *paths: str, ruff_path: str = "ruff")' in text) print("apply_ruff_lint passes ruff_path:", 'self._ruff_check_command("-", ruff_path=' in text) print("format_directory passes ruff_path:", 'self._ruff_check_command(str(directory), ruff_path=' in text) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/datamodel_code_generator/format.py` around lines 415 - 420, The helper _ruff_check_command currently hardcodes the literal "ruff" when callers omit ruff_path; change its implementation so that if ruff_path is the default "ruff" (or None) it calls self._find_ruff_path() and uses that resolved path instead, then build and return the command as before; keep the signature _ruff_check_command(self, *paths: str, ruff_path: str = "ruff") so apply_ruff_lint and format_directory can continue to omit ruff_path while apply_ruff_check_and_format can still pass an explicit value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/datamodel_code_generator/format.py`:
- Around line 415-420: The helper _ruff_check_command currently hardcodes the
literal "ruff" when callers omit ruff_path; change its implementation so that if
ruff_path is the default "ruff" (or None) it calls self._find_ruff_path() and
uses that resolved path instead, then build and return the command as before;
keep the signature _ruff_check_command(self, *paths: str, ruff_path: str =
"ruff") so apply_ruff_lint and format_directory can continue to omit ruff_path
while apply_ruff_check_and_format can still pass an explicit value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f6d9a3a-348f-40d2-89e3-e00066a475af
📒 Files selected for processing (2)
src/datamodel_code_generator/format.pytests/test_format.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/main/test_main_general.py (1)
1688-1698: Pin the output model type in these regressions.These checks are specifically about modular Pydantic runtime imports, but they currently rely on the project default still being a Pydantic model. Adding
--output-model-type pydantic_v2.BaseModelwould keep them from breaking on an unrelated default change.Also applies to: 1719-1729, 1741-1751
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/test_main_general.py` around lines 1688 - 1698, The tests like test_type_checking_imports_default_to_runtime_imports_for_modular_pydantic_ruff (and the similar blocks at the other two locations) assume a Pydantic output model but depend on the project default; update the run_main_and_assert calls (the extra_args list) to include "--output-model-type", "pydantic_v2.BaseModel" so the invocation explicitly pins the output model type and prevents unrelated default changes from breaking these regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/datamodel_code_generator/format.py`:
- Around line 206-218: The helper resolve_use_type_checking_imports currently
only considers defer_formatting but misses the in-memory multi-module generation
path; update resolve_use_type_checking_imports to accept a new boolean flag
(e.g., in_memory_multi_module) and change the final return so that when
in_memory_multi_module is true and is_pydantic_output and has_ruff it forces
runtime imports (i.e., return False / not TYPE_CHECKING), and then update
callers (generate(...) and Parser.parse()) to pass in_memory_multi_module=True
when output is None or when emitting multiple modules in-memory so Ruff cannot
move model imports into TYPE_CHECKING.
In `@tests/test_format.py`:
- Around line 172-186: Replace the hardcoded "/tmp/venv/bin/ruff" sentinel in
this test with a non-temp neutral constant and reuse it across the test file;
specifically, update the mock return_value for formatter._find_ruff_path and the
expected subprocess.run call to use a shared FAKE_RUFF_PATH (or similar) instead
of "/tmp/venv/bin/ruff" so the test no longer triggers Ruff S108 and avoids
repeating the literal, and update any other tests in this file that use the same
literal to reference the new constant.
---
Nitpick comments:
In `@tests/main/test_main_general.py`:
- Around line 1688-1698: The tests like
test_type_checking_imports_default_to_runtime_imports_for_modular_pydantic_ruff
(and the similar blocks at the other two locations) assume a Pydantic output
model but depend on the project default; update the run_main_and_assert calls
(the extra_args list) to include "--output-model-type", "pydantic_v2.BaseModel"
so the invocation explicitly pins the output model type and prevents unrelated
default changes from breaking these regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de4f5150-2f1d-498a-ab15-36733e31a307
📒 Files selected for processing (12)
src/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/_types/generate_config_dict.pysrc/datamodel_code_generator/_types/parser_config_dicts.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/config.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/parser/base.pytests/data/expected/main/input_model/config_class.pytests/main/test_main_general.pytests/main/test_public_api_signature_baseline.pytests/test_format.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/datamodel_code_generator/config.py
- src/datamodel_code_generator/main.py
- src/datamodel_code_generator/arguments.py
- src/datamodel_code_generator/_types/parser_config_dicts.py
- tests/data/expected/main/input_model/config_class.py
- tests/main/test_public_api_signature_baseline.py
- src/datamodel_code_generator/_types/generate_config_dict.py
- src/datamodel_code_generator/init.py
Generated by GitHub Actions
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli-reference/template-customization.md`:
- Around line 2348-2352: The paragraph is ambiguous about the default in the
multi-module Ruff + Pydantic case; update the text to explicitly state that the
new default behavior preserves runtime imports (i.e., Ruff will not move
generated model imports into TYPE_CHECKING blocks) for modular Pydantic output,
explain the runtime vs. type-checking trade-off, and call out that using
--use-type-checking-imports re-enables TYPE_CHECKING-only imports which restores
the old behavior but requires manual calls to model_rebuild() (or similar) for
cross-module references to work at runtime; mention the flags
--no-use-type-checking-imports and --use-type-checking-imports, TYPE_CHECKING,
Ruff, Pydantic, and model_rebuild() so readers can locate the relevant concepts.
In `@src/datamodel_code_generator/format.py`:
- Around line 217-218: The condition computing has_ruff and the return
expression currently treats both Formatter.RUFF_CHECK and Formatter.RUFF_FORMAT
the same; narrow this to only consider Formatter.RUFF_CHECK so format-only
setups don't trigger the workaround—update the has_ruff expression to test
Formatter.RUFF_CHECK (not RUFF_FORMAT) and keep the return using
is_multi_module_output and preserve_runtime_imports_for_multi_module_ruff
unchanged; look for the has_ruff variable and Formatter.RUFF_CHECK /
Formatter.RUFF_FORMAT in this function and change the boolean check accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eaea1a38-db6d-457a-8ccc-2c29b57474aa
⛔ Files ignored due to path filters (1)
docs/llms-full.txtis excluded by none and included by none
📒 Files selected for processing (14)
docs/cli-reference/index.mddocs/cli-reference/quick-reference.mddocs/cli-reference/template-customization.mdsrc/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/model/base.pysrc/datamodel_code_generator/model/pydantic_base.pysrc/datamodel_code_generator/model/pydantic_v2/dataclass.pysrc/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/prompt_data.pytests/data/expected/main/openapi/use_type_checking_imports_internal.pytests/main/test_main_general.pytests/test_format.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/main/test_main_general.py
- docs/cli-reference/quick-reference.md
- src/datamodel_code_generator/prompt_data.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/main/test_main_general.py (1)
1712-1723: Derive the package name fromoutput_dirinstead of hardcodingmodel.These runtime checks only need the generated package at
output_dir, but hardcoding"model"couples them to the fixture implementation and duplicates the same cleanup logic in both tests.♻️ Suggested cleanup
+ package_name = output_dir.name sys.path.insert(0, str(output_dir.parent)) importlib.invalidate_caches() try: - from model._internal import Result + Result = importlib.import_module(f"{package_name}._internal").Result result = Result.model_validate({"event": {"id": "abc"}}) assert result.event is not None assert result.event.__class__.__name__ == "Event" finally: sys.path.pop(0) - for name in [module for module in sys.modules if module == "model" or module.startswith("model.")]: + prefix = f"{package_name}." + for name in [module for module in sys.modules if module == package_name or module.startswith(prefix)]: del sys.modules[name]Also applies to: 1782-1793
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/test_main_general.py` around lines 1712 - 1723, The test currently hardcodes the package name "model" when inserting into sys.path, importing Result and cleaning sys.modules; change it to derive the package name from output_dir (e.g., pkg_name = output_dir.name or path-based derivation) and use that variable for the import and cleanup logic so the test targets the generated package instead of a hardcoded string; update the import statement that references Result.model_validate and the sys.modules cleanup list to use the derived pkg_name and also apply the same change to the similar block around lines 1782-1793.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/main/test_main_general.py`:
- Around line 1712-1723: The test currently hardcodes the package name "model"
when inserting into sys.path, importing Result and cleaning sys.modules; change
it to derive the package name from output_dir (e.g., pkg_name = output_dir.name
or path-based derivation) and use that variable for the import and cleanup logic
so the test targets the generated package instead of a hardcoded string; update
the import statement that references Result.model_validate and the sys.modules
cleanup list to use the derived pkg_name and also apply the same change to the
similar block around lines 1782-1793.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e799250c-44ba-4cb2-a2f1-d00b864e0f34
⛔ Files ignored due to path filters (1)
docs/llms-full.txtis excluded by none and included by none
📒 Files selected for processing (5)
docs/cli-reference/template-customization.mdsrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/parser/base.pytests/main/test_main_general.pytests/test_format.py
Contributor
Breaking Change AnalysisResult: No breaking changes detected Reasoning: This PR adds new CLI options (--use-type-checking-imports / --no-use-type-checking-imports) and configuration fields, all of which are optional and backward compatible. The default behavior change for multi-module Pydantic output with Ruff formatting is a bug fix that corrects runtime validation failures (issue #3038), not a breaking change. Previously, Ruff would move model imports into TYPE_CHECKING blocks, causing runtime failures because Pydantic couldn't find referenced models for validation. The new default keeps imports at runtime, producing correct working code. Users who want the old behavior can explicitly opt in with --use-type-checking-imports. This analysis was performed by Claude Code Action |
Contributor
|
🎉 Released in 0.55.0 This PR is now available in the latest release. See the release notes for details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--use-type-checking-imports/--no-use-type-checking-importsswitch for Ruff-based outputInvestigation
TYPE_CHECKINGblocks for pydantic #3038 with modular OpenAPI output formatted by RuffTYPE_CHECKING, which breaks runtime validation withoutmodel_rebuild()Testing
Fixes #3038
Summary by CodeRabbit
New Features
Documentation
Tests