-
Notifications
You must be signed in to change notification settings - Fork 176
Add hoist_classes
parameter in ctx.output_format
#1957
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.
Important
Looks good to me! 👍
Reviewed everything up to 57ba0c1 in 1 minute and 41 seconds. Click for details.
- Reviewed
619
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
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/jinja/src/evaluate_type/types.rs:289
- Draft comment:
The predefined type signature for 'baml::OutputFormat' does not include a parameter for 'map_style', even though RenderOptions::new expects one. Please add an entry (for example, using Type::merge(vec![Type::String, Type::None])) to keep the function’s interface in sync with the runtime API. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/jinja/src/evaluate_type/types.rs:294
- Draft comment:
The new 'hoist_classes' parameter is correctly merged into the union type; however, please double-check that the parameter ordering here matches the order expected by RenderOptions::new (which is: prefix, or_splitter, enum_value_prefix, always_hoist_enums, map_style, hoisted_class_prefix, hoist_classes) for consistency and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/jinja-runtime/src/output_format/types.rs:508
- Draft comment:
Typo in documentation: "you own't get any rendering!" should be "you won't get any rendering!". - Reason this comment was not posted:
Comment was on unchanged code.
4. engine/baml-lib/jinja-runtime/src/output_format/types.rs:777
- Draft comment:
Typographical mistakes in the comment: - 'iteraring' should be 'iterating'. - 'avoiaid' should be 'avoided'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is pointing out typographical errors in the code comments. While it is not asking the author to confirm or ensure anything, it is also not providing a suggestion or asking for a specific change in the code logic. It is purely informative about spelling mistakes, which does not align with the rules provided.
Workflow ID: wflow_kLfyJxmq6WBPix44
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 115a1ff in 1 minute and 56 seconds. Click for details.
- Reviewed
132
lines of code in1
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/jinja-runtime/src/output_format/types.rs:937
- Draft comment:
Consider replacing the manual newline trimming loop (lines 937–939) with a built‐in method such as output.trim_end_matches('\n') for conciseness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/jinja-runtime/src/output_format/types.rs:661
- Draft comment:
The inline condition for handling enums (lines 661–665) is complex. Consider refactoring or adding more detailed comments to clarify the intent. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/jinja-runtime/src/output_format/types.rs:876
- Draft comment:
Cloning the hoisted_classes set for iteration (line 876) is acceptable given mutation needs, but consider if precomputing hoisted enums could avoid cloning for improved clarity and efficiency. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_Wb4h9gkvyZyf9PPn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 85bfb09 in 2 minutes and 4 seconds. Click for details.
- Reviewed
294
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
6
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/jinja-runtime/src/output_format/types.rs:892
- Draft comment:
Avoid cloning the entire hoisted_classes set for iteration. Consider iterating over a copy to reduce unnecessary allocation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/jinja-runtime/src/output_format/types.rs:825
- Draft comment:
Improve error messaging for hoist_classes::Subset. The current error message may be unclear when classes are not found. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/jinja-runtime/src/output_format/types.rs:953
- Draft comment:
Consider using a built‐in trim (e.g. trim_end) instead of a manual while loop to remove trailing newlines for better performance and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-lib/jinja-runtime/src/output_format/types.rs:815
- Draft comment:
Add inline documentation for the new 'hoist_classes' parameter in RenderOptions and its usage in render(). This will help future maintainers understand the new behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. engine/baml-lib/jinja-runtime/src/output_format/types.rs:2304
- Draft comment:
The new test cases using raw multi‐line string literals are very comprehensive. Consider modularizing or centralizing expected outputs to ease future adjustments in formatting. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. engine/baml-lib/jinja-runtime/src/output_format/types.rs:610
- Draft comment:
Typographical suggestion: In this line, consider using "improving" instead of "improve" to maintain parallel structure with "saving tokens". - 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 the suggestion is technically correct from a grammatical perspective, it's a very minor change to documentation that doesn't affect functionality or clarity. The meaning is perfectly clear either way. The rules state not to make comments that are obvious or unimportant. The parallel structure would make the documentation slightly more polished and professional. Documentation quality does matter. While documentation quality matters, this is an extremely minor grammatical issue that doesn't impact understanding. The cost of the PR comment outweighs the tiny benefit. Delete this comment as it suggests a trivial grammatical change that doesn't meaningfully improve the code or documentation.
Workflow ID: wflow_3oC6RdTeQ0UgT2iA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 c5a7a1f in 2 minutes and 35 seconds. Click for details.
- Reviewed
301
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
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/jinja-runtime/src/output_format/types.rs:922
- Draft comment:
Consider replacing the manual newline-trimming loop with an idiomatic approach (e.g. using trim_end). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/jinja-runtime/src/output_format/types.rs:1005
- Draft comment:
Tests rely on exact whitespace and newline matching, which could be brittle. Consider normalizing or trimming output before comparison. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/jinja-runtime/src/output_format/types.rs:2275
- Draft comment:
The new hoist_classes parameter and related tests (e.g. render_hoisted_classes_with_prefix, hoist subset, hoist all) are comprehensive, and naming is consistent with builder methods. Good job! - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-lib/jinja-runtime/src/output_format/types.rs:970
- Draft comment:
The file contains numerous test cases, which is excellent for coverage; however, consider refactoring tests into separate modules or files for improved maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. engine/baml-lib/jinja-runtime/src/output_format/types.rs:742
- Draft comment:
Typo comment: In the format string on line 742, the syntax mixes shorthand named interpolation ({inner_str}
) with a positional placeholder ({}
). This can be confusing and error-prone. Consider using consistent placeholder syntax (e.g. either fully named or fully positional) to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_BgoLB4mLFRH23rT6
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -666,7 +889,7 @@ impl OutputFormatContent { | |||
|
|||
// once render_state.hoisted_enums is used, we shouldn't write to it again, hence why into_iter() over iter(). |
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.
The comment still refers to render_state.hoisted_enums
even though the code now uses render_ctx.hoisted_enums
. For consistency, please update the comment to reflect the correct variable name.
// once render_state.hoisted_enums is used, we shouldn't write to it again, hence why into_iter() over iter(). | |
// once render_ctx.hoisted_enums is used, we shouldn't write to it again, hence why into_iter() over iter(). |
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.
Important
Looks good to me! 👍
Reviewed c35ddb1 in 2 minutes and 21 seconds. Click for details.
- Reviewed
88
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
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/jinja-runtime/src/output_format/types.rs:175
- Draft comment:
New fieldhoist_classes: HoistClasses
is added to RenderOptions. Consider adding a doc comment that explains its behavior (e.g. Auto/All/Subset) and default value. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. engine/baml-lib/jinja-runtime/src/output_format/types.rs:243
- Draft comment:
The new constructorpub(crate) fn hoist_classes(hoist_classes: HoistClasses)
is introduced. Please add documentation/comments explaining its intended use and note that a more complete builder pattern might simplify future extensions. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. engine/baml-lib/jinja-runtime/src/output_format/types.rs:740
- Draft comment:
In the match arm forHoistClasses::Subset
, the format string mixes named and positional formatting. For clarity, consider using all named arguments (e.g., using{not_found}
) to avoid potential confusion. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. engine/baml-lib/jinja-runtime/src/output_format/types.rs:203
- Draft comment:
Typo: "paremeter" should be "parameter". - 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% While this is technically about a change in the diff (a new comment), typo fixes in comments are very minor and don't affect functionality. The rules say to only keep comments that require code changes. Fixing a typo in a comment is more like documentation cleanup than a required code change. The typo could make the code documentation less professional and harder to understand. Documentation quality does matter. While documentation quality matters, this particular typo is minor and doesn't significantly impact understanding. The meaning is still clear from context. Delete the comment. The typo is too minor to warrant a PR comment, as it doesn't require any functional code changes.
Workflow ID: wflow_mV6n8UZ3DRTej9Gn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed fa13211 in 56 seconds. Click for details.
- Reviewed
49
lines of code in3
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/jinja/src/evaluate_type/types.rs:307
- Draft comment:
The union type for the new 'hoist_classes' parameter is correctly defined as a merge of None, Bool, literal "auto", and list[string]. This looks consistent with similar parameters like 'always_hoist_enums'. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. engine/baml-lib/jinja/src/evaluate_type/types.rs:280
- Draft comment:
Ensure the order and candidates for output_format arguments remain consistent. 'hoist_classes' is now included alongside the other parameters. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. engine/baml-lib/jinja/src/evaluate_type/types.rs:311
- Draft comment:
The explicit literal value for "auto" is clear. Consider adding tests for valid cases (like hoist_classes=true and hoist_classes=["A", "B"]) if not already present. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_KeBj05od1VUQpECi
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 868878c in 1 minute and 38 seconds. Click for details.
- Reviewed
132
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
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. fern/03-reference/baml/prompt-syntax/output-format.mdx:185
- Draft comment:
The documentation for the boolean valuefalse
states it behaves the same as'auto'
. Consider clarifying that passingfalse
will cause only recursive classes to be hoisted (which is identical to the default behavior), so users are not confused about its purpose. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. fern/03-reference/baml/prompt-syntax/output-format.mdx:211
- Draft comment:
For consistency, consider wrapping schema names (e.g.Example
) in backticks in the rendered output example to clearly denote them as schema identifiers. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_sLyLDKUBu4orbLSB
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 7eab3c9 in 39 seconds. Click for details.
- Reviewed
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
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. fern/03-reference/baml/prompt-syntax/output-format.mdx:131
- Draft comment:
Good addition of the block to indicate the version requirement. Ensure the version notation '0.89+' is consistent with our versioning guidelines. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_ktAiG6exKJrHx0Aj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 08215e9 in 1 minute and 45 seconds. Click for details.
- Reviewed
39
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
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/jinja/src/evaluate_type/mod.rs:143
- Draft comment:
The TODO notes non-deterministic suggestion ordering caused by using a HashSet. Consider converting the valid_args to a sorted Vec or using a deterministic data structure (like BTreeSet) upfront so that suggestions remain consistent. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ui0B5GAu4WPjX8EV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
); | ||
|
||
// TODO: There's a bug here, suggestions are not always the same, maybe some |
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.
The test for unknown argument suggestions is commented out due to inconsistent ordering. Consider modifying the test to use order-insensitive or substring-based assertions to avoid brittle tests.
Important
Add
hoist_classes
parameter toctx.output_format
for controlling class hoisting in output schemas.hoist_classes
parameter toctx.output_format
inmod.rs
to control class hoisting.'auto'
,bool
,list[string]
.HoistClasses
enum intypes.rs
with variantsAll
,Subset
,Auto
.RenderOptions
to includehoist_classes
.test_expr.rs
andtypes.rs
to validatehoist_classes
behavior.output-format.mdx
to documenthoist_classes
parameter and its options.This description was created by
for 08215e9. You can customize this summary. It will automatically update as commits are pushed.