Implement tests for dynamic types #1343
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
antoniosarosi
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5da0bfa in 23 seconds
More details
- Looked at
54lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/schema-ast/src/parser/parse_type_builder_block.rs:65
- Draft comment:
Consider providing more context in the error message forBLOCK_LEVEL_CATCH_ALLto aid debugging. - Reason this comment was not posted:
Confidence changes required:50%
The error message forBLOCK_LEVEL_CATCH_ALLis clear and concise, but it might be helpful to include more context or details about the error to aid debugging.
Workflow ID: wflow_fVxqNXoeCbwPKjVk
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 06105d0 in 50 seconds
More details
- Looked at
67lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/parser-database/src/names/mod.rs:227
- Draft comment:
Typo in error message: 'defintions' should be 'definitions'. This typo is also present in the test filedynamic_types_validation_errors.bamlat line 122. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_87sGWXAoOuPrY8HC
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| // 56 | @@dynamic | ||
| // 57 | } | ||
| // | | ||
| // error: Error validating: Multiple dynamic defintions for type `DynamicClass` |
There was a problem hiding this comment.
Typo in error message: 'defintions' should be 'definitions'. This typo is also present in the code file mod.rs at line 227.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a3d33de in 26 seconds
More details
- Looked at
183lines of code in4files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:138
- Draft comment:
The current implementation uses a temporary solution to handle dynamic types by stripping the prefixast::DYNAMIC_TYPE_NAME_PREFIX. This is a workaround until scoping is implemented in the AST. Consider refactoring this once scoping is available. - Reason this comment was not posted:
Confidence changes required:50%
The code incycle.rshas a temporary solution for handling dynamic types by stripping a prefix. This is a workaround and should be noted for future refactoring.
2. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:182
- Draft comment:
The current implementation uses a temporary solution to handle dynamic types by checking for the prefixast::DYNAMIC_TYPE_NAME_PREFIX. This is a workaround until scoping is implemented in the AST. Consider refactoring this once scoping is available. - Reason this comment was not posted:
Confidence changes required:50%
The code incycle.rshas a temporary solution for handling dynamic types by checking for a prefix. This is a workaround and should be noted for future refactoring.
Workflow ID: wflow_MLGY7gF9HCfROle4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5ee049c in 50 seconds
More details
- Looked at
70lines of code in2files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. typescript/codemirror-lang-baml/src/index.ts:47
- Draft comment:
Ensure naming consistency: 'TypeBuilderKeyword' follows similar conventions as other keywords. Verify that this name is intentional. - Reason this comment was not posted:
Confidence changes required:0%
None
2. typescript/codemirror-lang-baml/src/index.ts:115
- Draft comment:
Including 'TypeBuilderDecl' in TestDecl content; confirm this is desired behavior for test declarations. - Reason this comment was not posted:
Confidence changes required:0%
None
3. typescript/codemirror-lang-baml/src/syntax.grammar:3
- Draft comment:
New 'TypeBuilderDecl' added to Decl production; verify that it integrates correctly with other declarations. - Reason this comment was not posted:
Confidence changes required:0%
None
4. typescript/codemirror-lang-baml/src/syntax.grammar:19
- Draft comment:
'TypeBuilderDecl' production allows a subset of declarations. Confirm that excluding alternatives like FunctionDecl is intentional. - Reason this comment was not posted:
Confidence changes required:0%
None
5. typescript/codemirror-lang-baml/src/index.ts:47
- Draft comment:
Added TypeBuilderDecl and TypeBuilderKeyword mappings. Confirm they align with the newly defined grammar tokens. - Reason this comment was not posted:
Confidence changes required:0%
None
6. typescript/codemirror-lang-baml/src/index.ts:60
- Draft comment:
DynamicDecl mapping mirrors other declarations; verify its consistent highlighting. - Reason this comment was not posted:
Confidence changes required:0%
None
7. typescript/codemirror-lang-baml/src/syntax.grammar:3
- Draft comment:
TypeBuilderDecl has been added to the top-level Decl union. Confirm that this fits the intended design, especially since DynamicDecl isn’t included here. - Reason this comment was not posted:
Confidence changes required:33%
None
8. typescript/codemirror-lang-baml/src/syntax.grammar:17
- Draft comment:
DynamicDecl is defined separately and isn’t part of the top-level Decl union. Ensure this is intentional to avoid ambiguity. - Reason this comment was not posted:
Confidence changes required:33%
None
9. typescript/codemirror-lang-baml/src/syntax.grammar:65
- Draft comment:
TestDecl was updated to accept TypeBuilderDecl. Verify that test blocks are meant to contain type builder declarations. - Reason this comment was not posted:
Confidence changes required:33%
None
10. typescript/codemirror-lang-baml/src/syntax.grammar:77
- Draft comment:
TypeBuilderKeyword is now included in the precedence block. Check for potential conflicts with IdentifierDecl resolution. - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_0ditYsEcm6YK45VK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e89e0f6 in 45 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. engine/baml-lib/baml/tests/validation_files/tests/dynamic_types_validation_errors.baml:139
- Draft comment:
Fix typo: 'defintions' should be 'definitions'. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. engine/baml-lib/parser-database/src/names/mod.rs:226
- Draft comment:
Fix typo in error message: change 'defintions' to 'definitions'. - Reason this comment was not posted:
Marked as duplicate.
3. engine/baml-lib/baml/tests/validation_files/tests/dynamic_types_validation_errors.baml:136
- Draft comment:
Fix spelling: use 'definitions' not 'defintions' in error message. - Reason this comment was not posted:
Confidence changes required:0%
None
4. engine/baml-lib/parser-database/src/names/mod.rs:227
- Draft comment:
Fix spelling: replace 'defintions' with 'definitions' in error message. - Reason this comment was not posted:
Confidence changes required:0%
None
Workflow ID: wflow_UX4n4sHEv9Z34ujx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bfe4f04 in 45 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:180
- Draft comment:
Ensure that using the 'dynamic_type' attribute intentionally bypasses enum value validation. This change accepts any string if 'dynamic_type' is set. Verify this is the desired behavior. - Reason this comment was not posted:
Comment did not seem useful.
2. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:181
- Draft comment:
Consider adding a comment explaining why a dynamic enum should accept any string when the 'dynamic_type' attribute is present. - Reason this comment was not posted:
Marked as duplicate.
3. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:181
- Draft comment:
Consider checking thedynamic_typeattribute first to bypass iterating over enum variants when the enum is dynamic. Adding a brief comment could clarify why a dynamic enum always passes. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_AYgP6MM8RuhHv53v
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 483363d in 37 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:1518
- Draft comment:
Good improvement replacing unwrap with proper error handling via .map_err. Consider factoring the conversion (format + JsError::new) into a helper to reduce repetition. - Reason this comment was not posted:
Confidence changes required:0%
None
2. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:1571
- Draft comment:
Similarly, error handling here is improved. Consider a helper for the repeated map_err conversion for better maintainability. - Reason this comment was not posted:
Confidence changes required:0%
None
3. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:1518
- Draft comment:
Good improvement replacing.unwrap()with.map_err(|e| JsError::new(format("{e:?}").as_str()))?to avoid panics. Consider adding more context to the error message (e.g. indicating that error occurred during test type builder retrieval) for easier debugging in the WASM context. - Reason this comment was not posted:
Confidence changes required:33%
None
4. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:1570
- Draft comment:
Nicely replaced the.unwrap()forcreate_ctxwith.map_err(...). For consistency and improved debugging, consider including context in the error message so the origin of the failure is clear. - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_c4HTtWi2F1pSrOle
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -177,7 +177,9 @@ impl ArgCoercer { | |||
| (FieldType::Enum(name), _) => match value { | |||
| BamlValue::String(s) => { | |||
| if let Ok(e) = ir.find_enum(name) { | |||
| if e.walk_values().any(|v| v.item.elem.0 == *s) { | |||
| if e.walk_values().any(|v| v.item.elem.0 == *s) | |||
| || e.item.attributes.get("dynamic_type").is_some() | |||
There was a problem hiding this comment.
Is this morally the same behavior that already exists for Class argument coersion in the case of dynamic types? https://github.com/BoundaryML/baml/pull/1343/files#diff-5fa0605b8093a86ff1b1b8e89032c02d46a052907bfa6dc38032c3ffc37aca02R238 I'm not sure, but it is. Just double-checking.
There was a problem hiding this comment.
Yes it's the same behavior. I think it makes sense, if you have a dynamic enum and add variants to it at runtime, you would not be able to pass those variants to functions if the coercer doesn't allow it. We might want to restrict both classes and enums to only values added at runtime or in type builder blocks, but for now this does the job.
Docs #1307
Discussion #1310
Important
Implement comprehensive testing and support for dynamic types in the BAML engine, including parsing, validation, and runtime handling.
to_baml_arg.rsby checking fordynamic_typeattribute.type_builderblocks inrepr.rsto handle dynamic type definitions within tests.type_builderblocks inlib.rs.dynamic_types.baml,dynamic_types_external_cycle_errors.baml,dynamic_types_internal_cycle_errors.baml,dynamic_types_parser_errors.baml, anddynamic_types_validation_errors.baml.run_type_builder_block_testintest_runtime.rsto test dynamic type handling.parse_type_expression_block.rsandparse_value_expression_block.rsto handledynamicandtype_builderblocks.parse_type_builder_block.rsfor parsingtype_builderblocks.RuntimeContextManagerandRuntimeContextto support dynamic type overrides.get_test_type_builderinruntime_interface.rsto retrieve type builders for tests.runtime_wasm/mod.rsto handle dynamic types in WASM environment.codemirror-lang-bamlandvscode-extto supporttype_builderanddynamicsyntax highlighting.This description was created by
for 483363d. It will automatically update as commits are pushed.