Repository navigation
feat(codegen): require naming_convention preserve-case - #3490
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces typed configuration for BAML generator blocks by adding ChangesNaming Convention Configuration Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c96d8f979b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+113
to
+114
| // Plumbed through but not yet consumed — see `NamingConvention` doc. | ||
| let _ = naming_convention; |
There was a problem hiding this comment.
When a generator specifies naming_convention "language", discover_generators accepts it and passes it through, but this code immediately discards the value, so Python output still preserves BAML identifiers instead of applying the documented snake_case/PascalCase rewrite. Until the Language branch is implemented, accepting it silently generates the wrong API surface for projects that opt into it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_cli/src/generate.rs (1)
158-232: 🏗️ Heavy liftMissing unit tests for
get_configper coding guidelines.
get_configis a pure slice-lookup with no external dependencies and is trivially unit-testable.discover_generatorsis harder (requires a liveProjectDatabase), butget_configat minimum deserves coverage. The coding guidelines require unit tests over integration tests where possible.♻️ Example unit test for `get_config`
#[cfg(test)] mod tests { use super::*; use baml_db::baml_compiler2_hir::item_tree::GeneratorConfigItem; fn make_item(key: &str, value: &str) -> GeneratorConfigItem { GeneratorConfigItem { key: key.into(), value: value.to_string(), } } #[test] fn get_config_returns_value_for_existing_key() { let items = vec![ make_item("output_type", "python/pydantic"), make_item("naming_convention", "\"preserve-case\""), ]; assert_eq!(get_config(&items, "output_type"), Some("python/pydantic".into())); assert_eq!(get_config(&items, "naming_convention"), Some("\"preserve-case\"".into())); } #[test] fn get_config_returns_none_for_missing_key() { let items = vec![make_item("output_type", "python/pydantic")]; assert_eq!(get_config(&items, "naming_convention"), None); } }As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible. Always run
cargo test --libif you changed any Rust code."🤖 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/src/generate.rs` around lines 158 - 232, The file lacks unit tests for the pure helper function get_config; add a #[cfg(test)] mod tests in the same file that imports baml_db::baml_compiler2_hir::item_tree::GeneratorConfigItem, implement a small helper (e.g., make_item) to construct GeneratorConfigItem, and add tests asserting get_config returns Some(value) for an existing key and None for a missing key (use the example cases for "output_type" and "naming_convention"); run cargo test --lib to verify.
🤖 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 `@baml_language/crates/baml_cli/src/generate.rs`:
- Around line 191-204: The parsing of raw_naming_convention should trim interior
whitespace after stripping surrounding quotes so values like "\" preserve-case
\"" parse correctly; update the code that sets naming_str (derived from
raw_naming_convention) to call .trim() after
.trim_matches('"').trim_matches('\'') before parse::<NamingConvention>(), and
apply the same .trim() fix to raw_output_dir handling to normalize whitespace;
update unit tests for get_config and discover_generators to cover quoted values
with extra spaces to prevent regressions.
---
Nitpick comments:
In `@baml_language/crates/baml_cli/src/generate.rs`:
- Around line 158-232: The file lacks unit tests for the pure helper function
get_config; add a #[cfg(test)] mod tests in the same file that imports
baml_db::baml_compiler2_hir::item_tree::GeneratorConfigItem, implement a small
helper (e.g., make_item) to construct GeneratorConfigItem, and add tests
asserting get_config returns Some(value) for an existing key and None for a
missing key (use the example cases for "output_type" and "naming_convention");
run cargo test --lib to verify.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a57b2f9c-428a-43dd-80b5-91f1f20e9abd
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
baml_language/crates/baml_cli/src/generate.rsbaml_language/languages/python/rust/codegen_python/Cargo.tomlbaml_language/languages/python/rust/codegen_python/src/lib.rsbaml_language/rig_tests/crates/basic_type_shapes/build.rsbaml_language/rig_tests/crates/docstrings_etc/build.rsbaml_language/rig_tests/crates/llm_functions/build.rsbaml_language/rig_tests/crates/type_shapes/build.rs
Merging this PR will improve performance by 20.86%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | engine_init_cost |
5.1 ms | 4.2 ms | +20.86% |
Comparing push-mxvouwwuslzk (cd5eac7) with canary (1b2b074)
Binary size checks passed✅ 7 passed
Generated by |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_cli/src/generate.rs (1)
188-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBug: non-Python generators now fail discovery instead of being skipped.
discover_generatorsparsesnaming_conventionfor every generator regardless ofoutput_type, but lowering's validation only requires it forpython/pydanticandpython/pydantic/v1. So a generator block with, e.g.,output_type "typescript"(or any future non-Python target) will:
- Pass lowering (no diagnostic, since validation is Python-gated),
- Hit
get_config(config, "naming_convention").unwrap_or_default()→ empty string,- Fail
naming_str.parse::<NamingConvention>()→Errreturned fromdiscover_generators,- Propagate via
?operator at line 79, aborting before reaching theother => skippingbranch at line 113.This breaks the contract that unsupported
output_types are warned-and-skipped, and means a Python-naming-convention requirement can no-op an entire codegen run if a user has even one non-Python generator block in theirbaml_src.The fix is to gate the parse on Python
output_typeso non-Python generators tolerate a missing/absentnaming_convention, mirroring lowering's gate.🛠️ Proposed fix
- // `naming_convention` is enforced as a hard error during - // CST → AST lowering (`MissingGeneratorProperty` / - // `InvalidGeneratorPropertyValue`), so by the time we get - // here the diagnostics gate above guarantees the field is - // present and valid. We still parse it defensively. - let raw_naming_convention = - get_config(config, "naming_convention").unwrap_or_default(); - let naming_str = raw_naming_convention - .trim() - .trim_matches('"') - .trim_matches('\''); - let naming_convention = naming_str.parse::<NamingConvention>().map_err(|_| { - anyhow!( - "generator '{}' has invalid naming_convention {naming_str:?} \ - (expected \"preserve-case\" or \"language\")", - generator_item.name - ) - })?; + // `naming_convention` is enforced as a hard error during + // CST → AST lowering for Python targets only + // (`MissingGeneratorProperty` / `InvalidGeneratorPropertyValue`). + // For non-Python targets we tolerate its absence so the + // dispatch match below can warn-and-skip cleanly. + let is_python = matches!( + output_type.as_str(), + "python/pydantic" | "python/pydantic/v1" + ); + let naming_convention = if is_python { + let raw = get_config(config, "naming_convention").unwrap_or_default(); + let naming_str = raw.trim().trim_matches('"').trim_matches('\'').trim(); + naming_str.parse::<NamingConvention>().map_err(|_| { + anyhow!( + "generator '{}' has invalid naming_convention {naming_str:?} \ + (expected \"preserve-case\" or \"language\")", + generator_item.name + ) + })? + } else { + // Placeholder; not consumed by non-Python codegen paths. + NamingConvention::PreserveCase + };🤖 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/src/generate.rs` around lines 188 - 225, discover_generators currently parses naming_convention for every generator which makes non-Python generators fail; change the logic so you only call get_config/config parsing and parse::<NamingConvention>() when the resolved output_type is a Python target (“python/pydantic” or “python/pydantic/v1”); for other output_type values skip the parse and assign the same safe default NamingConvention the code expects (so the later generators.push(GeneratorDef { ... naming_convention, ... }) works), using the existing symbols: discover_generators, output_type, get_config, NamingConvention, and generator_item.name to locate and gate the parsing.
🧹 Nitpick comments (3)
baml_language/crates/baml_cli/src/generate.rs (1)
195-205: 💤 Low valueNote: no separate comment needed — whitespace handling looks correct.
Past review's whitespace concern (
.trim()after quote stripping) appears resolved here: trimming first, then stripping quotes, is the inverse but functionally equivalent for typical inputs without inner-quote whitespace. If you adopt the gating fix above, please also append a trailing.trim()after.trim_matches('\'')for symmetry with values like" preserve-case "(inner whitespace) — same pattern recommended inlower_cst.rs::validate_generator_properties.🤖 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/src/generate.rs` around lines 195 - 205, The trimming sequence for raw_naming_convention should also trim any whitespace that remains after stripping quotes—update the chain that builds naming_str (which currently calls raw_naming_convention.trim().trim_matches('"').trim_matches('\'');) to append a final .trim() so the resulting naming_str is fully trimmed before parsing into NamingConvention and error reporting for generator_item.name.baml_language/crates/baml_compiler2_ast/src/lower_cst.rs (2)
1303-1361: ⚡ Quick winAdd unit tests for
validate_generator_properties.This helper has multiple branches (non-Python skip, missing key, invalid value, quoted/unquoted value variants) and is a hard-error producer at lowering time. As per coding guidelines (
Prefer writing Rust unit tests over integration tests where possible), please cover the happy-paths and each diagnostic branch with#[cfg(test)]tests in this module — at minimum:
- non-Python
output_typeproduces no diagnostics regardless ofnaming_convention,- missing
naming_conventionforpython/pydanticemitsMissingGeneratorProperty,naming_convention "snake"emitsInvalidGeneratorPropertyValuewith the offendingvalue,- both
"preserve-case"and"language"(with surrounding whitespace) are accepted.As per coding guidelines:
Prefer writing Rust unit tests over integration tests where possible.🤖 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_compiler2_ast/src/lower_cst.rs` around lines 1303 - 1361, Add #[cfg(test)] unit tests in the same module to exercise validate_generator_properties: create small test cases that call validate_generator_properties with various config_items and a dummy block_span and assert the produced diags contains the expected LoweringDiagnostic variants. Specifically add tests for (1) a non-Python output_type (e.g. "rust/serde") that yields no diagnostics regardless of naming_convention, (2) python/pydantic output_type with no naming_convention that yields LoweringDiagnostic::MissingGeneratorProperty, (3) python/pydantic with naming_convention "snake" that yields LoweringDiagnostic::InvalidGeneratorPropertyValue with value "snake", and (4) python/pydantic with naming_convention values "preserve-case" and "language" (including surrounding whitespace/quotes) that produce no diagnostics; locate and call validate_generator_properties and assert on diags contents and spans accordingly.
1320-1325: The Python output type list is already complete and synchronized—no missing variants found.The codebase only supports
"python/pydantic"and"python/pydantic/v1", as confirmed by theGeneratorOutputTypeenum inbaml-types/src/generator.rs. Both dispatch points—lower_cst.rs(line 1322) andgenerate.rs(line 108)—use identical match patterns, so there is no drift. No planned variants (e.g.,python/pydantic/v2) appear in the codebase.However, consider extracting a shared
is_python_output_type()helper to centralize this check and reduce the risk of future desynchronization as new backends are added.🤖 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_compiler2_ast/src/lower_cst.rs` around lines 1320 - 1325, Duplicate checks for Python output types exist (matches!(output_type.as_str(), "python/pydantic" | "python/pydantic/v1")) in lower_cst.rs and generate.rs; extract a shared helper named is_python_output_type(output_type: &str) that returns the boolean and replace the inline matches! calls in functions that reference output_type (e.g., the block around naming_convention in lower_cst.rs and the dispatch in generate.rs) to centralize the logic and avoid future desynchronization.
🤖 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 `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 1316-1346: The trimming order for generator properties is wrong:
when extracting output_type (from find("output_type")) and the naming_convention
value (the local value inside the match for find("naming_convention")), strip
surrounding quote characters first and then call trim() to remove inner
whitespace (i.e., change the current
.trim().trim_matches('"').trim_matches('\'') sequence to strip quotes first then
.trim()); update both the output_type extraction and the naming_convention value
computation so they mirror the fix applied in generate.rs and will avoid false
InvalidGeneratorPropertyValue diagnostics while keeping the existing
LoweringDiagnostic::MissingGeneratorProperty handling.
---
Outside diff comments:
In `@baml_language/crates/baml_cli/src/generate.rs`:
- Around line 188-225: discover_generators currently parses naming_convention
for every generator which makes non-Python generators fail; change the logic so
you only call get_config/config parsing and parse::<NamingConvention>() when the
resolved output_type is a Python target (“python/pydantic” or
“python/pydantic/v1”); for other output_type values skip the parse and assign
the same safe default NamingConvention the code expects (so the later
generators.push(GeneratorDef { ... naming_convention, ... }) works), using the
existing symbols: discover_generators, output_type, get_config,
NamingConvention, and generator_item.name to locate and gate the parsing.
---
Nitpick comments:
In `@baml_language/crates/baml_cli/src/generate.rs`:
- Around line 195-205: The trimming sequence for raw_naming_convention should
also trim any whitespace that remains after stripping quotes—update the chain
that builds naming_str (which currently calls
raw_naming_convention.trim().trim_matches('"').trim_matches('\'');) to append a
final .trim() so the resulting naming_str is fully trimmed before parsing into
NamingConvention and error reporting for generator_item.name.
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 1303-1361: Add #[cfg(test)] unit tests in the same module to
exercise validate_generator_properties: create small test cases that call
validate_generator_properties with various config_items and a dummy block_span
and assert the produced diags contains the expected LoweringDiagnostic variants.
Specifically add tests for (1) a non-Python output_type (e.g. "rust/serde") that
yields no diagnostics regardless of naming_convention, (2) python/pydantic
output_type with no naming_convention that yields
LoweringDiagnostic::MissingGeneratorProperty, (3) python/pydantic with
naming_convention "snake" that yields
LoweringDiagnostic::InvalidGeneratorPropertyValue with value "snake", and (4)
python/pydantic with naming_convention values "preserve-case" and "language"
(including surrounding whitespace/quotes) that produce no diagnostics; locate
and call validate_generator_properties and assert on diags contents and spans
accordingly.
- Around line 1320-1325: Duplicate checks for Python output types exist
(matches!(output_type.as_str(), "python/pydantic" | "python/pydantic/v1")) in
lower_cst.rs and generate.rs; extract a shared helper named
is_python_output_type(output_type: &str) that returns the boolean and replace
the inline matches! calls in functions that reference output_type (e.g., the
block around naming_convention in lower_cst.rs and the dispatch in generate.rs)
to centralize the logic and avoid future desynchronization.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cbca26dc-8d93-432d-beff-71a1be9c419c
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
baml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.bamlbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snap.newbaml_language/languages/python/rust/codegen_python/Cargo.tomlbaml_language/languages/python/rust/codegen_python/src/lib.rsbaml_language/rig_tests/crates/basic_type_shapes/build.rsbaml_language/rig_tests/crates/docstrings_etc/build.rsbaml_language/rig_tests/crates/llm_functions/build.rsbaml_language/rig_tests/crates/type_shapes/build.rs
✅ Files skipped from review due to trivial changes (3)
- baml_language/languages/python/rust/codegen_python/Cargo.toml
- baml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.baml
- baml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snap.new
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/rig_tests/crates/basic_type_shapes/build.rs
- baml_language/rig_tests/crates/docstrings_etc/build.rs
- baml_language/rig_tests/crates/llm_functions/build.rs
- baml_language/languages/python/rust/codegen_python/src/lib.rs
Comment on lines
+1316
to
+1346
| let output_type = find("output_type") | ||
| .map(|c| c.value.trim().trim_matches('"').trim_matches('\'').to_string()) | ||
| .unwrap_or_default(); | ||
|
|
||
| // `naming_convention` is required for Python targets only — other | ||
| // backends do not yet have a casing rewrite story. | ||
| let is_python = matches!(output_type.as_str(), "python/pydantic" | "python/pydantic/v1"); | ||
| if !is_python { | ||
| return; | ||
| } | ||
|
|
||
| let allowed = r#""preserve-case" or "language""#; | ||
| match find("naming_convention") { | ||
| None => { | ||
| diags.push(LoweringDiagnostic::MissingGeneratorProperty { | ||
| generator_name: gen_name.to_string(), | ||
| property: "naming_convention", | ||
| message: format!( | ||
| "generator `{gen_name}` is missing required property `naming_convention` \ | ||
| (expected {allowed})" | ||
| ), | ||
| span: block_span, | ||
| }); | ||
| } | ||
| Some(item) => { | ||
| let value = item | ||
| .value | ||
| .trim() | ||
| .trim_matches('"') | ||
| .trim_matches('\'') | ||
| .to_string(); |
Contributor
There was a problem hiding this comment.
Trim inner whitespace after stripping quotes (consistency with generate.rs).
c.value.trim().trim_matches('"').trim_matches('\'') trims outer whitespace first, then strips one layer of quotes — so a value like " preserve-case " becomes preserve-case and fails the value != "preserve-case" check, producing a confusing InvalidGeneratorPropertyValue for what is logically a valid value. The same issue was raised on generate.rs and resolved by trimming after quote-stripping; please mirror that here for both output_type and naming_convention so the lowering diagnostic and the CLI accept the same set of inputs.
🛡️ Proposed fix
let output_type = find("output_type")
- .map(|c| c.value.trim().trim_matches('"').trim_matches('\'').to_string())
+ .map(|c| {
+ c.value
+ .trim()
+ .trim_matches('"')
+ .trim_matches('\'')
+ .trim()
+ .to_string()
+ })
.unwrap_or_default();
@@
Some(item) => {
let value = item
.value
.trim()
.trim_matches('"')
.trim_matches('\'')
+ .trim()
.to_string();🤖 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_compiler2_ast/src/lower_cst.rs` around lines 1316 -
1346, The trimming order for generator properties is wrong: when extracting
output_type (from find("output_type")) and the naming_convention value (the
local value inside the match for find("naming_convention")), strip surrounding
quote characters first and then call trim() to remove inner whitespace (i.e.,
change the current .trim().trim_matches('"').trim_matches('\'') sequence to
strip quotes first then .trim()); update both the output_type extraction and the
naming_convention value computation so they mirror the fix applied in
generate.rs and will avoid false InvalidGeneratorPropertyValue diagnostics while
keeping the existing LoweringDiagnostic::MissingGeneratorProperty handling.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_codegen_types/src/naming.rs (1)
9-17: ⚡ Quick winAdd a unit test for
strumround-trip serialization.The
EnumString/Displayimpls are load-bearing (they're whatlower_cst.rsparses from BAML source), but there's no unit test verifying the kebab-case round-trip. A single#[cfg(test)]block would catch any strum attribute typo before it reaches integration tests.✅ Suggested unit test
+#[cfg(test)] +mod tests { + use super::*; + use std::str::FromStr; + + #[test] + fn naming_convention_roundtrip() { + assert_eq!(NamingConvention::PreserveCase.to_string(), "preserve-case"); + assert_eq!(NamingConvention::Language.to_string(), "language"); + assert_eq!( + NamingConvention::from_str("preserve-case").unwrap(), + NamingConvention::PreserveCase + ); + assert_eq!( + NamingConvention::from_str("language").unwrap(), + NamingConvention::Language + ); + assert!(NamingConvention::from_str("preserve_case").is_err()); + assert!(NamingConvention::from_str("PreserveCase").is_err()); + } +}As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible."
🤖 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_codegen_types/src/naming.rs` around lines 9 - 17, Add a unit test that verifies strum serialization/deserialization round-trip for the NamingConvention enum: create a #[cfg(test)] mod with a test (e.g., test_naming_convention_strum_roundtrip) that iterates the NamingConvention variants (PreserveCase, Language), calls to_string() on each to get the kebab-case form and then parses back using <NamingConvention as std::str::FromStr>::from_str (or NamingConvention::from_str) asserting the parsed value equals the original; place the test in the same file so it fails fast if the strum attributes on NamingConvention are wrong.
🤖 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_codegen_types/src/naming.rs`:
- Around line 9-17: Add a unit test that verifies strum
serialization/deserialization round-trip for the NamingConvention enum: create a
#[cfg(test)] mod with a test (e.g., test_naming_convention_strum_roundtrip) that
iterates the NamingConvention variants (PreserveCase, Language), calls
to_string() on each to get the kebab-case form and then parses back using
<NamingConvention as std::str::FromStr>::from_str (or
NamingConvention::from_str) asserting the parsed value equals the original;
place the test in the same file so it fails fast if the strum attributes on
NamingConvention are wrong.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b90ca664-3e67-4f9f-8640-be50e4e220f5
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__01_lexer__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__02_parser__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
baml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_codegen_types/Cargo.tomlbaml_language/crates/baml_codegen_types/src/lib.rsbaml_language/crates/baml_codegen_types/src/naming.rsbaml_language/crates/baml_compiler2_ast/Cargo.tomlbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.bamlbaml_language/languages/python/rust/codegen_python/src/lib.rsbaml_language/rig_tests/crates/basic_type_shapes/build.rsbaml_language/rig_tests/crates/docstrings_etc/build.rsbaml_language/rig_tests/crates/llm_functions/build.rsbaml_language/rig_tests/crates/type_shapes/build.rs
✅ Files skipped from review due to trivial changes (4)
- baml_language/crates/baml_codegen_types/Cargo.toml
- baml_language/crates/baml_compiler2_ast/Cargo.toml
- baml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.baml
- baml_language/crates/baml_codegen_types/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- baml_language/rig_tests/crates/basic_type_shapes/build.rs
- baml_language/rig_tests/crates/docstrings_etc/build.rs
- baml_language/crates/baml_compiler2_ast/src/lower_cst.rs
- baml_language/rig_tests/crates/type_shapes/build.rs
- baml_language/rig_tests/crates/llm_functions/build.rs
- baml_language/crates/baml_cli/src/generate.rs
- baml_language/languages/python/rust/codegen_python/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@baml_language/crates/baml_cli/src/generate.rs`:
- Around line 185-197: The code unconditionally parses naming_convention with
parse::<NamingConvention>().expect(...) which can panic for non-Python
generators; change this so parsing is guarded by the generator's output_type
(only parse when output_type indicates python/pydantic) or make
raw_naming_convention an Option and defer/validate parsing at the use site.
Locate the get_config call that produces raw_naming_convention and the parse
into NamingConvention and either (A) wrap the parse behind an output_type check
(use the generator's output_type field) or (B) store Option<NamingConvention>
(keep raw string from get_config) and parse/validate when code generation
actually needs a NamingConvention, returning a proper error instead of
panicking. Ensure references to NamingConvention, get_config, naming_convention,
and output_type are updated accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2bf609e0-3879-4952-a3d4-e36859a86941
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__01_lexer__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__02_parser__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
baml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_codegen_types/Cargo.tomlbaml_language/crates/baml_codegen_types/src/lib.rsbaml_language/crates/baml_codegen_types/src/naming.rsbaml_language/crates/baml_compiler2_ast/Cargo.tomlbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/semantic_tokens/generator.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords1.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords2.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/enum/enum_is_valid.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/generators/v1.bamlbaml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.bamlbaml_language/languages/python/rust/codegen_python/src/lib.rsbaml_language/rig_tests/crates/basic_type_shapes/build.rsbaml_language/rig_tests/crates/docstrings_etc/build.rsbaml_language/rig_tests/crates/llm_functions/build.rsbaml_language/rig_tests/crates/type_shapes/build.rs
✅ Files skipped from review due to trivial changes (7)
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords2.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/enum/enum_is_valid.baml
- baml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/semantic_tokens/generator.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/generators/v1.baml
- baml_language/crates/baml_codegen_types/src/naming.rs
- baml_language/crates/baml_compiler2_ast/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (7)
- baml_language/rig_tests/crates/type_shapes/build.rs
- baml_language/rig_tests/crates/docstrings_etc/build.rs
- baml_language/rig_tests/crates/llm_functions/build.rs
- baml_language/crates/baml_compiler2_ast/src/lower_cst.rs
- baml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rs
- baml_language/crates/baml_codegen_types/Cargo.toml
- baml_language/languages/python/rust/codegen_python/src/lib.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
baml_language/crates/baml_compiler2_hir/src/item_tree.rs (1)
540-553: ⚡ Quick winAdd a unit test for
collect_generator_spans.No unit test covers the new method. A minimal round-trip test (construct a mock
GeneratorDef, callcollect_generator_spans, assert both maps contain the expected spans) would guard against regressions when the span-collection logic evolves.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible."
🤖 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_compiler2_hir/src/item_tree.rs` around lines 540 - 553, Add a unit test for collect_generator_spans that constructs an ItemTreeSourceMap, creates a mock ast::GeneratorDef with a specific span and a few config_items each with known spans, calls collect_generator_spans(&mut source_map, id, &gen_def), and then asserts that source_map.generator_block_spans contains the generator id mapped to gen_def.span and that source_map.generator_config_item_spans contains the same id mapped to the vector of config item spans; place the test in the same module (e.g., a #[cfg(test)] mod tests in item_tree.rs) and use a deterministic LocalItemId<GeneratorMarker> for the lookup.baml_language/sdks/python/rust/codegen_python/src/lib.rs (1)
20-20: 💤 Low value
OutputTypere-export is unnecessary here.
NamingConventionbelongs in this crate's public surface because callers need it to construct theto_source_codeargument.OutputType, however, is a CLI-level discriminant (python/pydanticvs.python/pydantic/v1) and is not used anywhere incodegen_python— no parameter, no dispatch, no field. Exporting it from a codegen crate conflates the generator selection concern with the code emission concern.♻️ Proposed cleanup
-pub use baml_codegen_types::{NamingConvention, OutputType}; +pub use baml_codegen_types::NamingConvention;Callers that need
OutputTypecan import it directly frombaml_codegen_types.🤖 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/sdks/python/rust/codegen_python/src/lib.rs` at line 20, Remove the unnecessary re-export of OutputType from the crate public surface: keep only pub use of NamingConvention in lib.rs (remove OutputType from the pub use statement) so this crate exposes only the types it actually uses (e.g., NamingConvention for to_source_code). Update any callers that relied on baml_language::codegen_python::OutputType to import OutputType directly from baml_codegen_types instead.
🤖 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_compiler2_hir/src/item_tree.rs`:
- Around line 540-553: Add a unit test for collect_generator_spans that
constructs an ItemTreeSourceMap, creates a mock ast::GeneratorDef with a
specific span and a few config_items each with known spans, calls
collect_generator_spans(&mut source_map, id, &gen_def), and then asserts that
source_map.generator_block_spans contains the generator id mapped to
gen_def.span and that source_map.generator_config_item_spans contains the same
id mapped to the vector of config item spans; place the test in the same module
(e.g., a #[cfg(test)] mod tests in item_tree.rs) and use a deterministic
LocalItemId<GeneratorMarker> for the lookup.
In `@baml_language/sdks/python/rust/codegen_python/src/lib.rs`:
- Line 20: Remove the unnecessary re-export of OutputType from the crate public
surface: keep only pub use of NamingConvention in lib.rs (remove OutputType from
the pub use statement) so this crate exposes only the types it actually uses
(e.g., NamingConvention for to_source_code). Update any callers that relied on
baml_language::codegen_python::OutputType to import OutputType directly from
baml_codegen_types instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a8972c99-cc08-4471-9c36-e2edd33162a8
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__01_lexer__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__02_parser__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
baml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_codegen_types/Cargo.tomlbaml_language/crates/baml_codegen_types/src/lib.rsbaml_language/crates/baml_codegen_types/src/naming.rsbaml_language/crates/baml_codegen_types/src/output_type.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/semantic_tokens/generator.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords1.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords2.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/enum/enum_is_valid.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/generators/v1.bamlbaml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.bamlbaml_language/sdk_tests/crates/basic_type_shapes/build.rsbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdks/python/rust/codegen_python/src/lib.rs
✅ Files skipped from review due to trivial changes (5)
- baml_language/crates/baml_codegen_types/Cargo.toml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords2.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/enum/enum_is_valid.baml
- baml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/generators/v1.baml
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_lsp2_actions_tests/test_files/semantic_tokens/generator.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords1.baml
- baml_language/crates/baml_codegen_types/src/naming.rs
- baml_language/crates/baml_cli/src/generate.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
baml_language/crates/baml_codegen_types/src/generator_fields.rs (1)
22-30: ⚡ Quick winAdd strum parse/display round-trip unit tests.
There are no tests confirming that
"preserve-case"and"language"round-trip correctly throughEnumString/Display. As per coding guidelines, prefer Rust unit tests. A small test block would guard against future strum version drift or attribute typos:✅ Suggested test block
+#[cfg(test)] +mod tests { + use super::*; + use std::str::FromStr; + + #[test] + fn naming_convention_round_trips() { + assert_eq!(NamingConvention::from_str("preserve-case").unwrap(), NamingConvention::PreserveCase); + assert_eq!(NamingConvention::PreserveCase.to_string(), "preserve-case"); + assert_eq!(NamingConvention::from_str("language").unwrap(), NamingConvention::Language); + assert_eq!(NamingConvention::Language.to_string(), "language"); + } + + #[test] + fn output_type_round_trips() { + assert_eq!(OutputType::from_str("python/pydantic").unwrap(), OutputType::PythonPydantic); + assert_eq!(OutputType::PythonPydantic.to_string(), "python/pydantic"); + assert_eq!(OutputType::from_str("python/pydantic/v1").unwrap(), OutputType::PythonPydanticV1); + } +}🤖 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_codegen_types/src/generator_fields.rs` around lines 22 - 30, Add unit tests that verify strum's EnumString and Display round-trip for NamingConvention: for each variant (PreserveCase and Language) call strum::EnumString to parse the kebab-case strings "preserve-case" and "language" into NamingConvention and assert equality, then call to_string() (Display) on those enum values and assert the result matches the original kebab-case string; place tests in the same module as the NamingConvention enum (e.g., within a #[cfg(test)] mod tests) so future changes to the #[strum(serialize_all = "kebab-case")] attribute will be caught.baml_language/sdks/python/rust/codegen_python/src/lib.rs (1)
20-20: 💤 Low valueConsider dropping the
OutputTypere-export from the Python codegen crate.
OutputTypeis never used in this file; re-exporting a backend-selection enum from a backend-specific crate creates a coupling smell — callers can import it directly frombaml_codegen_types.-pub use baml_codegen_types::{NamingConvention, OutputType}; +pub use baml_codegen_types::NamingConvention;🤖 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/sdks/python/rust/codegen_python/src/lib.rs` at line 20, Remove the unnecessary re-export of OutputType from lib.rs to avoid backend coupling: change the pub use baml_codegen_types::{NamingConvention, OutputType}; to only re-export NamingConvention (pub use baml_codegen_types::NamingConvention;), and if any callers relied on this crate for OutputType, update their imports to use baml_codegen_types::OutputType directly.
🤖 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_codegen_types/src/generator_fields.rs`:
- Around line 22-30: Add unit tests that verify strum's EnumString and Display
round-trip for NamingConvention: for each variant (PreserveCase and Language)
call strum::EnumString to parse the kebab-case strings "preserve-case" and
"language" into NamingConvention and assert equality, then call to_string()
(Display) on those enum values and assert the result matches the original
kebab-case string; place tests in the same module as the NamingConvention enum
(e.g., within a #[cfg(test)] mod tests) so future changes to the
#[strum(serialize_all = "kebab-case")] attribute will be caught.
In `@baml_language/sdks/python/rust/codegen_python/src/lib.rs`:
- Line 20: Remove the unnecessary re-export of OutputType from lib.rs to avoid
backend coupling: change the pub use baml_codegen_types::{NamingConvention,
OutputType}; to only re-export NamingConvention (pub use
baml_codegen_types::NamingConvention;), and if any callers relied on this crate
for OutputType, update their imports to use baml_codegen_types::OutputType
directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36a9fc54-08dd-4c44-adf1-c62031e1b85c
⛔ Files ignored due to path filters (4)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__01_lexer__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__02_parser__valid_generator.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/generator/baml_tests__broken_syntax__generator__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
baml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_codegen_types/Cargo.tomlbaml_language/crates/baml_codegen_types/src/generator_fields.rsbaml_language/crates/baml_codegen_types/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/semantic_tokens/generator.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords1.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords2.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/enum/enum_is_valid.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/generators/v1.bamlbaml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.bamlbaml_language/sdk_tests/crates/basic_type_shapes/build.rsbaml_language/sdk_tests/crates/docstrings_etc/build.rsbaml_language/sdk_tests/crates/llm_functions/build.rsbaml_language/sdk_tests/crates/type_shapes/build.rsbaml_language/sdks/python/rust/codegen_python/src/lib.rs
✅ Files skipped from review due to trivial changes (7)
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords2.baml
- baml_language/crates/baml_codegen_types/Cargo.toml
- baml_language/sdk_tests/crates/basic_type_shapes/build.rs
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/enum/enum_is_valid.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/semantic_tokens/generator.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/generators/v1.baml
- baml_language/crates/baml_tests/projects/broken_syntax/generator/valid_generator.baml
🚧 Files skipped from review as they are similar to previous changes (8)
- baml_language/sdk_tests/crates/llm_functions/build.rs
- baml_language/sdk_tests/crates/docstrings_etc/build.rs
- baml_language/crates/baml_compiler2_hir/src/builder.rs
- baml_language/sdk_tests/crates/type_shapes/build.rs
- baml_language/crates/baml_compiler2_hir/src/item_tree.rs
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/generator_keywords1.baml
- baml_language/crates/baml_cli/src/generate.rs
- baml_language/crates/baml_codegen_types/src/lib.rs
This was referenced Jun 2, 2026
9 of 10 tasks
5 tasks done
This branch was previously deployed
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
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 by CodeRabbit
New Features
naming_conventionconfiguration to control how identifiers are handled in generated codeBug Fixes