-
Notifications
You must be signed in to change notification settings - Fork 170
Add dynamic and streaming-mode data to FieldType #2004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 076ed75 in 2 minutes and 28 seconds. Click for details.
- Reviewed
602
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/baml-types/src/ir_type/mod.rs:738
- Draft comment:
Consider adding clarifying comments in the partialize function’s optional-wrapping branch (lines 739–748) to explain the metadata swap logic and why the inner metadata is reset before wrapping the type as optional. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. engine/baml-lib/baml-types/src/baml_value.rs:450
- Draft comment:
Typographical issue: the conditionenum_value == enum_value
compares the same variable to itself. It appears that this may be a mistake; consider verifying if it should instead compare theenum_value
fromBamlValueWithMeta::Enum
with a corresponding value from theFieldType::Enum
variant. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% Looking at the code more carefully, the leftenum_value
comes from the BamlValueWithMeta::Enum variant (line 449) while the rightenum_value
is a pattern variable from the FieldType::Enum match pattern (line 450). These are actually two different variables that happen to have the same name, not the same variable being compared to itself. The comment is incorrect. Could there be a reason why comparing enum values is important for type checking? Could removing this check break something? Yes, comparing enum values is important - an enum type check should verify both the enum name and value match. Removing the value comparison would make the type checking too permissive. The comment is incorrect - this is not comparing a variable to itself, but rather comparing two different enum values that happen to have the same variable name. The comparison is intentional and correct.
3. engine/baml-lib/baml-types/src/ir_type/mod.rs:196
- Draft comment:
Typographical issue: It looks like there's a missing comma after the block for theUnionTypeViewGeneric::OneOf(field_types)
match arm. Please add a trailing comma after the closing brace to separate it correctly from the following arm. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While consistent comma style can be nice, this is a very minor stylistic issue. The code works perfectly fine without the trailing comma. The Rust compiler doesn't require it. This kind of minor style nitpick creates noise in PR reviews and distracts from more important issues. Additionally, many teams use automated formatters like rustfmt which would handle this automatically if it was part of their style guide. Could inconsistent comma style lead to git diff noise in future changes? Could it violate team style guidelines? Even if there are style guidelines, this is better handled by automated tools like rustfmt rather than manual PR comments. The potential git diff impact is minimal for such a small style issue. This comment should be deleted as it's too minor of a style issue to warrant a PR comment. Style consistency is better handled by automated tools.
Workflow ID: wflow_CLpG6BwG2WRw5Zxw
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
BamlValueWithMeta::Enum(enum_name, _, _) => match field_type { | ||
FieldType::Enum(enm, _) => enum_name == enm, | ||
BamlValueWithMeta::Enum(enum_name, enum_value, _) => match field_type { | ||
FieldType::Enum { name: enm, .. } => enum_name == enm && enum_value == enum_value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: In the Enum
arm of is_type
, the condition uses enum_value == enum_value
which always evaluates to true. Likely this should compare the stored enum value against an expected value from the field type (or simply be removed).
FieldType::Enum { name: enm, .. } => enum_name == enm && enum_value == enum_value, | |
FieldType::Enum { name: enm, .. } => enum_name == enm, |
Important
Add dynamic and streaming-mode data to
FieldType
by modifyingTypeGeneric
and updatingpartialize
function to handle new fields.dynamic
andmode
fields toEnum
andClass
variants inTypeGeneric
inmod.rs
.StreamingMode
enum inmod.rs
to represent streaming states.partialize
function inmod.rs
to handle newdynamic
andmode
fields.is_type()
inbaml_value.rs
to checkdynamic
andmode
fields.partialize_helper()
inmod.rs
to apply streaming behavior based ondynamic
andmode
.partialize
function inmod.rs
to verify handling ofdynamic
andmode
fields.r#enum()
andclass()
inbuilder.rs
to initializedynamic
andmode
fields.This description was created by
for 076ed75. You can customize this summary. It will automatically update as commits are pushed.