feat: auto-disable black/isort with UserWarning when not installed#3391
feat: auto-disable black/isort with UserWarning when not installed#3391mvanhorn wants to merge 2 commits into
Conversation
…oxudaxi#3058) Wrap _get_black()/_get_isort() availability checks in CodeFormatter.__init__ with a new _is_formatter_available() helper. When black or isort is requested but not importable, emit a UserWarning and fall back to the built-in formatter rather than crashing with ImportError. Adds six pytest cases covering each fallback scenario.
Merging this PR will improve performance by 17.34%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_complex_refs |
2.1 s | 1.8 s | +19.05% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.9 s | 3.3 s | +18.67% |
| ⚡ | WallTime | test_perf_all_options_enabled |
5.8 s | 4.9 s | +18.55% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,148.8 ms | 972.3 ms | +18.16% |
| ⚡ | WallTime | test_perf_openapi_large |
3.1 s | 2.6 s | +18.08% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
2 s | 1.7 s | +17.59% |
| ⚡ | WallTime | test_perf_deep_nested |
5.5 s | 4.7 s | +17.27% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
848.3 ms | 724.7 ms | +17.05% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.8 s | 3.2 s | +16.55% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.8 s | 2.4 s | +15.42% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
2.1 s | 1.8 s | +14.43% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mvanhorn:feat/3058-formatter-auto-fallback (9b0bcc7) with main (bcd0b3c)
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. ↩
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3391 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 138 138
Lines 29274 29338 +64
Branches 3504 3510 +6
===========================================
+ Hits 29274 29333 +59
- Misses 0 4 +4
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds runtime detection for missing ChangesFormatter dependency validation
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/datamodel_code_generator/format.py (1)
189-202: 💤 Low valueConsider catching broader exceptions in availability check.
The function currently catches only
ImportError. If a formatter package is installed but has a broken__init__.pyor other import-time errors, those exceptions will propagate. While the current behavior is reasonable (broken installation should fail loudly), you might want to catch all exceptions and returnFalsefor more graceful degradation.🛡️ Optional defensive enhancement
def _is_formatter_available(formatter: Formatter) -> bool: """Return True if the external package required by *formatter* can be imported.""" if formatter.value == "black": try: import black # noqa: F401, PLC0415 - except ImportError: + except Exception: # noqa: BLE001 return False elif formatter.value == "isort": try: import isort # noqa: F401, PLC0415 - except ImportError: + except Exception: # noqa: BLE001 return False return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/datamodel_code_generator/format.py` around lines 189 - 202, The availability check in _is_formatter_available currently only catches ImportError when importing formatter packages ('black' and 'isort'); change those except ImportError clauses to except Exception so any import-time error (e.g., broken __init__ or runtime error during import) is caught and the function returns False, keeping the rest of the logic intact for the Formatter enum branches in _is_formatter_available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/datamodel_code_generator/format.py`:
- Around line 442-452: The warn message for missing Black should not claim a
built-in fallback unconditionally: in the block that checks "if Formatter.BLACK
in formatters and not _is_formatter_available(Formatter.BLACK)" update the logic
to first remove BLACK from formatters, then check "if not
EXTERNAL_FORMATTERS.intersection(formatters) and Formatter.BUILTIN not in
formatters" — if that condition is true emit the current message saying you're
falling back to the built-in formatter and then append BUILTIN to formatters;
otherwise emit a message matching the isort style such as "black is not
installed; the black formatting step will be skipped." Use the existing symbols
Formatter.BLACK, Formatter.BUILTIN, EXTERNAL_FORMATTERS, _is_formatter_available
and warn to implement this conditional messaging and behavior.
---
Nitpick comments:
In `@src/datamodel_code_generator/format.py`:
- Around line 189-202: The availability check in _is_formatter_available
currently only catches ImportError when importing formatter packages ('black'
and 'isort'); change those except ImportError clauses to except Exception so any
import-time error (e.g., broken __init__ or runtime error during import) is
caught and the function returns False, keeping the rest of the logic intact for
the Formatter enum branches in _is_formatter_available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e414cc6d-ddf1-4bd2-9415-2b5757d7d8cf
📒 Files selected for processing (2)
src/datamodel_code_generator/format.pytests/test_format.py
| # Auto-disable black/isort when not installed, fall back to built-in. | ||
| if Formatter.BLACK in formatters and not _is_formatter_available(Formatter.BLACK): | ||
| warn( | ||
| "black is not installed; falling back to the built-in formatter. " | ||
| "Install black or pass formatters=[Formatter.BUILTIN] to suppress this warning.", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) | ||
| formatters = [f for f in formatters if f is not Formatter.BLACK] | ||
| if Formatter.BUILTIN not in formatters and not EXTERNAL_FORMATTERS.intersection(formatters): | ||
| formatters = [*formatters, Formatter.BUILTIN] |
There was a problem hiding this comment.
Warning message claims builtin fallback unconditionally, but fallback only occurs when no external formatters remain.
When black is unavailable but other external formatters like isort or ruff remain active, the warning at line 445 says "falling back to the built-in formatter", but Formatter.BUILTIN is only added at line 452 when not EXTERNAL_FORMATTERS.intersection(formatters) (i.e., when no external formatters remain). This misleads users into thinking builtin formatting will be used when it won't.
For example, with formatters=[Formatter.BLACK, Formatter.ISORT] and black not installed:
- Warning claims: "falling back to the built-in formatter"
- Actual behavior:
BLACKremoved,ISORTremains active, no builtin fallback added
The isort warning at line 456 correctly states "the isort formatting step will be skipped" without claiming fallback. Consider aligning the black warning message to either:
- Match the
isortpattern: "black is not installed; the black formatting step will be skipped." - Make the fallback claim conditional: emit "falling back to the built-in formatter" only when line 452 actually adds
BUILTIN.
♻️ Suggested fix: conditional warning message
if Formatter.BLACK in formatters and not _is_formatter_available(Formatter.BLACK):
+ formatters = [f for f in formatters if f is not Formatter.BLACK]
+ will_add_builtin = Formatter.BUILTIN not in formatters and not EXTERNAL_FORMATTERS.intersection(formatters)
+ if will_add_builtin:
+ warn(
+ "black is not installed; falling back to the built-in formatter. "
+ "Install black or pass formatters=[Formatter.BUILTIN] to suppress this warning.",
+ UserWarning,
+ stacklevel=2,
+ )
+ formatters = [*formatters, Formatter.BUILTIN]
+ else:
+ warn(
+ "black is not installed; the black formatting step will be skipped. "
+ "Install black or use formatters=[Formatter.BUILTIN] for built-in formatting.",
+ UserWarning,
+ stacklevel=2,
+ )
- warn(
- "black is not installed; falling back to the built-in formatter. "
- "Install black or pass formatters=[Formatter.BUILTIN] to suppress this warning.",
- UserWarning,
- stacklevel=2,
- )
- formatters = [f for f in formatters if f is not Formatter.BLACK]
- if Formatter.BUILTIN not in formatters and not EXTERNAL_FORMATTERS.intersection(formatters):
- formatters = [*formatters, Formatter.BUILTIN]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Auto-disable black/isort when not installed, fall back to built-in. | |
| if Formatter.BLACK in formatters and not _is_formatter_available(Formatter.BLACK): | |
| warn( | |
| "black is not installed; falling back to the built-in formatter. " | |
| "Install black or pass formatters=[Formatter.BUILTIN] to suppress this warning.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) | |
| formatters = [f for f in formatters if f is not Formatter.BLACK] | |
| if Formatter.BUILTIN not in formatters and not EXTERNAL_FORMATTERS.intersection(formatters): | |
| formatters = [*formatters, Formatter.BUILTIN] | |
| # Auto-disable black/isort when not installed, fall back to built-in. | |
| if Formatter.BLACK in formatters and not _is_formatter_available(Formatter.BLACK): | |
| formatters = [f for f in formatters if f is not Formatter.BLACK] | |
| will_add_builtin = Formatter.BUILTIN not in formatters and not EXTERNAL_FORMATTERS.intersection(formatters) | |
| if will_add_builtin: | |
| warn( | |
| "black is not installed; falling back to the built-in formatter. " | |
| "Install black or pass formatters=[Formatter.BUILTIN] to suppress this warning.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) | |
| formatters = [*formatters, Formatter.BUILTIN] | |
| else: | |
| warn( | |
| "black is not installed; the black formatting step will be skipped. " | |
| "Install black or use formatters=[Formatter.BUILTIN] for built-in formatting.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/datamodel_code_generator/format.py` around lines 442 - 452, The warn
message for missing Black should not claim a built-in fallback unconditionally:
in the block that checks "if Formatter.BLACK in formatters and not
_is_formatter_available(Formatter.BLACK)" update the logic to first remove BLACK
from formatters, then check "if not EXTERNAL_FORMATTERS.intersection(formatters)
and Formatter.BUILTIN not in formatters" — if that condition is true emit the
current message saying you're falling back to the built-in formatter and then
append BUILTIN to formatters; otherwise emit a message matching the isort style
such as "black is not installed; the black formatting step will be skipped." Use
the existing symbols Formatter.BLACK, Formatter.BUILTIN, EXTERNAL_FORMATTERS,
_is_formatter_available and warn to implement this conditional messaging and
behavior.
|
@mvanhorn |
…of auto-fallback Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reworked per your direction in d9f9bc1: the automatic fallback is gone. When black or isort is requested but not importable, the CodeFormatter now raises a clear ImportError that names the missing formatter and suggests installing it or passing --formatters builtin. Tests updated accordingly (106 passing locally, ruff clean). This also keeps the door open for making builtin the default later without any hidden behavior switch. |
2 participants
Summary
Auto-disables
blackandisortformatters with aUserWarningwhen they are not installed, falling back to the built-in formatter instead of crashing withImportError.Why this matters
Closes #3058. Users who consume
datamodel-code-generatorvia the Python API (generate_dynamic_models) in lightweight environments should not be forced to installblackandisort. Today, the default formatter list includes both, causing a hardImportErrorif either is absent. With this change, missing formatters are detected eagerly inCodeFormatter.__init__, a clearUserWarningis emitted, and the built-in formatter activates automatically - zero behaviour change for installs that haveblack/isortpresent.Changes
src/datamodel_code_generator/format.py: Adds_is_formatter_available(formatter)helper that checks importability without raising. InCodeFormatter.__init__, calls this helper forFormatter.BLACKandFormatter.ISORTbefore attempting to load them; emitsUserWarningand drops the unavailable formatter from the active list. If no external formatters remain,Formatter.BUILTINis automatically added.tests/test_format.py: Six new parametric tests covering black-missing, isort-missing, both-missing (BUILTIN fallback), both-present (no warning), explicit-BUILTIN (no warning), and black-only-missing-with-isort-still-active scenarios.Fixes #3058
Summary by CodeRabbit
Bug Fixes
--formatters builtin(no silent fallback when externals are missing).Tests