Repository navigation
Conversation
Co-authored-by: Dhilan Shah <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a typed UnsupportedNamingConvention error and Result-returning SDK APIs, updates harness and CLI to handle and report codegen errors, validates generator naming_convention during discovery (rejecting ChangesPython/Pydantic naming convention enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Co-authored-by: Dhilan Shah <[email protected]>
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_cli/tests/exit_code_e2e.rs (1)
232-276: 💤 Low valueOptional: Remove unnecessary
default_client_modefield.The
default_client_mode "sync"field on line 242 is not required for this test and is not part of the tested behavior (naming_convention validation). The existing helpercreate_project_with_generator(lines 44-54) doesn't include this field. Consider removing it for clarity, or document why it's included if it serves a specific purpose.♻️ Simplified test fixture
create_project( tmp.path(), r#"generator target { output_type "python/pydantic" output_dir "../" - default_client_mode "sync" naming_convention "language" }🤖 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 `@baml_language/crates/baml_cli/tests/exit_code_e2e.rs` around lines 232 - 276, In the test generate_python_pydantic_language_naming_convention_returns_diagnostic, remove the unnecessary default_client_mode "sync" field from the generator fixture created via create_project (it is not required for the naming_convention validation being tested and differs from the helper create_project_with_generator); update the generator block passed to create_project so it only includes the relevant keys (target, output_type, output_dir, naming_convention) to keep the fixture minimal and consistent with other tests.
🤖 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.
Nitpick comments:
In `@baml_language/crates/baml_cli/tests/exit_code_e2e.rs`:
- Around line 232-276: In the test
generate_python_pydantic_language_naming_convention_returns_diagnostic, remove
the unnecessary default_client_mode "sync" field from the generator fixture
created via create_project (it is not required for the naming_convention
validation being tested and differs from the helper
create_project_with_generator); update the generator block passed to
create_project so it only includes the relevant keys (target, output_type,
output_dir, naming_convention) to keep the fixture minimal and consistent with
other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 013d2222-ef73-438f-8caf-59428390c83d
📒 Files selected for processing (4)
baml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_cli/tests/exit_code_e2e.rsbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rs
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!( | ||
| f, | ||
| "sdkgen_python_pydantic2 only supports naming_convention = PreserveCase (got {})", |
There was a problem hiding this comment.
I think this is the final user-facing diagnostic? We should make this more legible
generator python/pydanticrequires settingnaming_convention preserve-case
| let naming_convention_expected = match output_type { | ||
| Some(OutputType::PythonPydantic | OutputType::PythonPydanticV1) => { | ||
| r#""preserve-case""# | ||
| } | ||
| Some(OutputType::TypescriptNode) | None => r#""preserve-case" or "language""#, | ||
| }; |
There was a problem hiding this comment.
language is never allowed today, i.e. TypescriptNode should have same behavior
| })); | ||
| match codegen_result { | ||
| Ok(output) => { | ||
| Ok(Ok(output)) => { |
There was a problem hiding this comment.
i'm missing something here, why are we going from Result<codegen output, codegen error> to Result<Result<...>, codegen error>?
Co-authored-by: Dhilan Shah <[email protected]>
Pull Request Template
Thanks for taking the time to fill out this pull request!
Issue Reference
The error
The following BAML crashed before this change when generated with the compiler2 CLI:
Pre-fix reproduction with
cargo run -p baml_cli --bin baml-cli -- generate --from /tmp/tmp.WSMWmSKIpBexited through Rust panic while running under Cargo:Root cause
baml_language/crates/baml_cli/src/generate.rs::discover_generatorsparsednaming_conventionwith a target-agnostic expected-value list that accepted and advertised both"preserve-case"and"language"for every generator. The python/pydantic backend inbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rs::to_source_code_internalonly supportsPreserveCaseand asserted when it receivedLanguage, so CLI validation let an unsupported value reach a panic path.The SDK test harness in
baml_language/sdk_tests/harness_setup/src/python_pydantic2.rs::codegen_fixturealso called that backend directly and needed to handle the new fallible result instead of only catching panics.The fix
python/pydanticandpython/pydantic/v1, so missingnaming_conventionguidance lists only"preserve-case"for those targets.naming_convention "language"for python/pydantic targets withDiagnosticId::InvalidGeneratorPropertyValueand a primary span on the offending generator config item.UnsupportedNamingConventionerror instead of asserting internally.Verification
Pre-fix reproduction was run first and produced the panic shown above.
Post-fix manual reproduction:
Commands run from
baml_language/:Snapshot tests were covered by the full
cargo test; there were no snapshot updates to accept.CodeRabbit:
Because
CODERABBIT_API_KEYis missing from this environment, I could not authenticate or runcoderabbit review --plain --base canary.Environment note: the repository setup script installed the main mise-managed toolchain but exited while installing
cargo-shear@latestwith cargo exit code 101 in this environment. I continued after trustingbaml_language/mise.tomland running the SDK setup needed by plaincargo test.Changes
See The fix above.
Testing
Screenshots
N/A
PR Checklist
Additional Notes
All code changes are inside
baml_language/. The legacyengine/directory was not modified.Summary by CodeRabbit
naming_conventionwere tightened to only allow the supported option and to surface precise error locations.