Skip to content

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

Merged
merged 10 commits into from
May 19, 2025

Conversation

antoniosarosi
Copy link
Contributor

@antoniosarosi antoniosarosi commented May 16, 2025

Important

Add hoist_classes parameter to ctx.output_format for controlling class hoisting in output schemas.

  • Behavior:
    • Adds hoist_classes parameter to ctx.output_format in mod.rs to control class hoisting.
    • Supports values: 'auto', bool, list[string].
    • Recursive classes are always hoisted.
  • Types:
    • Introduces HoistClasses enum in types.rs with variants All, Subset, Auto.
    • Updates RenderOptions to include hoist_classes.
  • Tests:
    • Adds tests in test_expr.rs and types.rs to validate hoist_classes behavior.
  • Documentation:
    • Updates output-format.mdx to document hoist_classes parameter and its options.

This description was created by Ellipsis for 08215e9. You can customize this summary. It will automatically update as commits are pushed.

Copy link

vercel bot commented May 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2025 10:59pm

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% None

Workflow ID: wflow_Wb4h9gkvyZyf9PPn

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 Ellipsis 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().
Copy link
Contributor

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.

Suggested change
// 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().

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 field hoist_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% <= threshold 50% None
2. engine/baml-lib/jinja-runtime/src/output_format/types.rs:243
  • Draft comment:
    The new constructor pub(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% <= threshold 50% None
3. engine/baml-lib/jinja-runtime/src/output_format/types.rs:740
  • Draft comment:
    In the match arm for HoistClasses::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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% None

Workflow ID: wflow_KeBj05od1VUQpECi

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 value false states it behaves the same as 'auto'. Consider clarifying that passing false 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% <= threshold 50% None

Workflow ID: wflow_sLyLDKUBu4orbLSB

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% None

Workflow ID: wflow_ktAiG6exKJrHx0Aj

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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 Ellipsis 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
Copy link
Contributor

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.

@antoniosarosi antoniosarosi added this pull request to the merge queue May 19, 2025
Merged via the queue into canary with commit 42ee507 May 19, 2025
12 of 13 checks passed
@antoniosarosi antoniosarosi deleted the antonio/hoist-classes branch May 19, 2025 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant