Skip to content

Reapply "LLM function co:wqmposition (#1722)" (#1758) #1759

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 2 commits into from
Apr 10, 2025
Merged

Conversation

imalsogreg
Copy link
Contributor

@imalsogreg imalsogreg commented Apr 10, 2025

This reverts commit e25d147, and reintroduces Exprs.

Add several new syntactic forms to BAML, allowing the definition of functions whose bodies are expressions that may compose calls to other expression-bodied functions as well as the existing LLM functions.

The changes to the language in this PR are not meant to be used in production BAML apps. This is foundation work for a future, user-facing release. All syntax introduced here is very likely to change.

Closes #1707

Needs cleanup and more tests.


Important

Reintroduces expression functions in BAML, adding foundational support for complex function compositions and updates across the codebase to handle new syntax and types.

  • Behavior:
    • Reintroduces expression functions in BAML, allowing function bodies to compose calls to other expression-bodied functions.
    • Not intended for production use; foundational for future releases.
  • IR Changes:
    • Adds ExprFunctionNode and ExprFunctionWalker in ir_helpers/mod.rs.
    • Updates IntermediateRepr to include expr_fns and toplevel_assignments.
  • Type System:
    • Introduces Arrow type in field_type/mod.rs for function types.
    • Updates type coercion and checking to handle new Arrow type.
  • Validation:
    • Adds validation for expression functions in expr_fns.rs and expr_typecheck.rs.
  • Code Generation:
    • Updates Python, Ruby, and TypeScript code generation to handle new function types, though Arrow types are not yet supported in generated definitions.
  • Testing:
    • Adds tests for expression functions in validation_files/expr/.
  • Misc:
    • Updates flake.nix and flake.lock for dependency management.
    • Adjusts build scripts and configurations in flake.nix and package.json.

This description was created by Ellipsis for 33c071e. It will automatically update as commits are pushed.

Copy link

vercel bot commented Apr 10, 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 Apr 10, 2025 5:43am

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.

❌ Changes requested. Reviewed everything up to 22908a8 in 5 minutes and 30 seconds

More details
  • Looked at 6660 lines of code in 87 files
  • Skipped 1 files when reviewing.
  • Skipped posting 63 drafted comments based on config settings.
1. engine/language_client_codegen/src/go/generate_types.rs:60
  • Draft comment:
    Avoid panicking on FieldType::Arrow – even if arrow types aren’t supported, consider returning an error instead of panic to simplify future integration.
  • 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/language_client_codegen/src/python/generate_types.rs:255
  • Draft comment:
    Consider handling FieldType::Arrow gracefully in the conversion rather than using todo!.
  • 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/language_client_codegen/src/ruby/field_type.rs:60
  • Draft comment:
    Consistently flag Arrow types using a todo! or error message; consider standardized error handling across language generators.
  • 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%
    Both todo!() and panic!() serve similar purposes - they both halt execution with a message. todo!() specifically indicates incomplete implementation while panic!() indicates a runtime error condition. Since this is about preventing an invalid use case rather than marking incomplete work, panic!() might be more semantically appropriate. However, this is a relatively minor distinction that doesn't materially impact functionality.
    The change is more about code style than fixing a real issue. The existing todo!() already prevents Arrow types from being used, so the suggested change doesn't solve any actual problems.
    While panic!() might be marginally more semantically appropriate, the benefit is minimal and the existing code is already functional and clear in its intent.
    This comment should be deleted as it suggests a change that provides minimal value and doesn't address any real issues with the code.
4. engine/language_client_codegen/src/typescript/mod.rs:668
  • Draft comment:
    Review union type rendering logic – make sure that the joining and conversion to string preserves intended parentheses.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
5. flake.nix:23
  • Draft comment:
    Ensure that using 'nixos-24.05' meets your requirements; consider adding comments if a stable channel is intended.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
6. typescript/nextjs-plugin/src/index.ts:48
  • Draft comment:
    The fallback for Next.js version sets default to 14; ensure this backward compatibility is well documented.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
7. typescript/playground-common/src/baml_wasm_web/EventListener.tsx:80
  • Draft comment:
    Ensure that error handling in the event listener (e.g. clearHighlights) robustly handles all cases. Otherwise, code looks good.
  • Reason this comment was not posted:
    Confidence changes required: 10% <= threshold 50%
    None
8. typescript/vscode-ext/packages/vscode/src/extension.ts:144
  • Draft comment:
    Consider verifying disposal of statusBarItem and other resources with null checks to avoid potential exceptions on deactivation.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
9. typescript/vscode-ext/packages/vscode/src/panels/WebviewPanelHost.ts:168
  • Draft comment:
    Ensure that the webview content generated with nonce and proper resource URIs is updated when resource paths change.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
10. typescript/nextjs-plugin/src/index.ts:91
  • Draft comment:
    Double-check reactivity in webpack config updates (e.g., in watchOptions and module.rules) to ensure consistency across environments.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
11. engine/language_client_codegen/src/go/generate_types.rs:49
  • Draft comment:
    Arrow types currently panic with 'Generation is not supported with expr fns'. Consider providing a clearer error or alternative conversion for expression functions.
  • 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.
12. engine/language_client_codegen/src/python/generate_types.rs:255
  • Draft comment:
    In Python type conversion, Arrow types trigger a todo!(); consider providing a more user-friendly error message than a panic.
  • 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.
13. engine/language_client_codegen/src/ruby/field_type.rs:60
  • Draft comment:
    Arrow types in Ruby conversion use a todo!(); ensure that unsupported expression functions are documented or handled gracefully.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is suggesting to ensure that unsupported expression functions are documented or handled gracefully. This is a general suggestion and not a specific code suggestion or request for a test. It seems to be asking the author to ensure something, which violates the rules.
14. engine/language_client_codegen/src/ruby/generate_types.rs:159
  • Draft comment:
    Ruby partial type conversion does not support Arrow types; consider handling them or providing a clear error message.
  • 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.
15. engine/language_client_codegen/src/typescript/mod.rs:677
  • Draft comment:
    Arrow types in TypeScript conversion trigger a todo!() placeholder. Document these limitations if expr functions become user‐facing.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
    The comment is informative, suggesting documentation of limitations if a certain feature becomes user-facing. It doesn't provide a specific code suggestion or ask for a test to be written. It seems to be more of a note or reminder rather than actionable feedback.
16. flake.nix:21
  • Draft comment:
    Ensure that the toolchain configuration using LLVM 17 remains compatible with upstream changes. Verify that all toolchain inputs are up-to-date.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to ensure compatibility and verify updates, which falls under the rule of not asking the author to confirm or ensure things. It doesn't provide a specific code suggestion or point out a specific issue.
17. integ-tests/typescript/package.json:10
  • Draft comment:
    Test timeout increased to 60000 ms; verify that this value is appropriate for your CI environment.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify if the test timeout value is appropriate for their CI environment. This falls under asking the author to ensure the behavior is intended, which is against the rules. Therefore, this comment should be removed.
18. typescript/fiddle-frontend/next.config.mjs:8
  • Draft comment:
    Next.js configuration properly includes a WebAssembly loader. Ensure experimental features remain compatible with production environments.
  • 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.
19. typescript/nextjs-plugin/src/index.ts:3
  • Draft comment:
    The plugin extracts the Next.js version; consider caching the computed version to reduce redundant file system accesses.
  • 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.
20. typescript/playground-common/src/baml_wasm_web/EventListener.tsx:25
  • Draft comment:
    Ensure cleanup of event listeners on component unmount to prevent memory leaks; verify that atom callbacks do not retain stale state.
  • 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.
21. typescript/playground-common/src/shared/baml-project-panel/playground-panel/prompt-preview/test-panel/test-runner.ts:88
  • Draft comment:
    Test-runner calls clearHighlights on errors and at test completion; consider debouncing these UI updates if they impact performance.
  • 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%
    The comment is speculative - it suggests debouncing "if they impact performance" but provides no evidence that there is actually a performance problem. The clearHighlights calls happen at distinct logical points (errors, test completion) rather than in rapid succession. Debouncing could actually be harmful as it would delay clearing highlights when they should be cleared immediately.
    The comment does identify a potential optimization. Multiple clearHighlights calls in quick succession could cause unnecessary UI updates.
    Without evidence of an actual performance problem, and given that the clear calls happen at logically distinct points, premature optimization would add complexity for unclear benefit.
    Delete this comment as it is speculative and suggests optimization without evidence of an actual problem.
22. typescript/vscode-ext/packages/vscode/src/extension.ts:62
  • Draft comment:
    Extension logs are verbose; consider gating debug logs behind a verbosity flag before production deployment.
  • 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.
23. typescript/vscode-ext/packages/vscode/src/panels/WebviewPanelHost.ts:168
  • Draft comment:
    WebviewPanelHost generates HTML using a nonce. Confirm that nonce generation is secure and that localResourceRoots restrict resource loading appropriately.
  • 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.
24. typescript/vscode-ext/packages/vscode/src/panels/WebviewPanelHost.ts:242
  • Draft comment:
    In the 'jumpToFile' case, ensure robust error handling if the referenced file does not exist.
  • 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.
25. typescript/vscode-ext/packages/vscode/src/panels/WebviewPanelHost.ts:216
  • Draft comment:
    Verify that the pathRewrite regex in the proxy middleware handles all edge cases properly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to verify that the regex handles all edge cases, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested.
26. .github/workflows/primary.yml:152
  • Draft comment:
    Minor typographical fix: Removed the trailing whitespace from the 'macos-latest' entry in the matrix include section for the build-cli job.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative as it only mentions a minor typographical fix without suggesting any action or improvement. It doesn't provide any actionable feedback or ask for confirmation on an important change.
27. .github/workflows/primary.yml:173
  • Draft comment:
    Ensure that the file ends with a newline. The diff indicates that a newline was missing at the end of the file, which is a minor formatting improvement.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and suggests a minor formatting improvement. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't address a potential issue or bug in the code.
28. engine/baml-lib/baml-core/src/ir/ir_helpers/mod.rs:200
  • Draft comment:
    Typo: The comment in the FieldType::Arrow match arm currently says "and no in the return type check", which is likely a typo. Consider changing it to "and not in the return type check" for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While the comment is technically correct about the typo, it's a very minor issue in a code comment that doesn't affect functionality. The meaning is still reasonably clear even with the typo. The rules state we should not make purely informative comments or comments that are obvious/unimportant. A typo in a comment seems to fall into this category.
    The typo could potentially cause confusion for future developers trying to understand the covariance/contravariance behavior being described. Code comments are part of documentation and clarity is important.
    While documentation clarity is important, this typo is minor enough that most readers would understand the intended meaning from context. The comment is still understandable and the fix is trivial.
    The comment should be deleted as it points out a very minor typo that doesn't significantly impact understanding and doesn't require a formal PR comment to fix.
29. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:360
  • Draft comment:
    Consider capitalizing 'JSON' in the error message on coercing function types (line 360). It might be clearer to say: "A JSON value may not be coerced into a function type".
  • 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 comment is technically correct (JSON is conventionally capitalized), this is an extremely minor stylistic suggestion that doesn't impact functionality. The error message is already clear and understandable. The rules state not to make purely informative comments or comments that are obvious/unimportant.
    The capitalization of JSON is a widely accepted convention in technical writing. The current lowercase version could be seen as slightly unprofessional.
    While proper capitalization is good practice, this is too minor of an issue to warrant a PR comment. It doesn't affect functionality or code quality in any meaningful way.
    Delete this comment as it's too minor and purely stylistic, violating the rule about not making obvious or unimportant comments.
30. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:364
  • Draft comment:
    There's a minor typographical error in the unreachable! message (line 364): 'FieldType::Constrainted' should likely be 'FieldType::Constrained'.
  • Reason this comment was not posted:
    Comment was on unchanged code.
31. engine/baml-lib/baml-core/src/ir/repr.rs:1957
  • Draft comment:
    Typographical error: In the comment for the Prompt enum, 'stirng' should be corrected to 'string'.
  • 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.
32. engine/baml-lib/baml-types/src/baml_value.rs:720
  • Draft comment:
    Typographical error: In the comment above line 720, 'classs' should be corrected to 'classes' (or 'class's', as appropriate).
  • 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.
33. engine/baml-lib/baml-types/src/value_expr.rs:15
  • Draft comment:
    Typographical error detected: In the comment for the Numeric variant, 'Repred as a string, but guaranteed to be a number.' should likely be corrected to 'Represented as a string, but guaranteed to be a number.'
  • 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.
34. engine/baml-lib/baml/tests/validation_files/expr/expr_fn.baml:11
  • Draft comment:
    Minor: The file does not end with a newline character. Please add a newline at the end of file to match common conventions.
  • 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%
    Missing newlines at end of file are a common convention and can cause issues with some tools. However, this is a very minor issue that would likely be caught by linters or formatters. The rules state not to make comments about obvious or unimportant issues. This seems like a trivial formatting issue.
    Missing newlines can cause problems with some version control systems and tools, so this could be a legitimate issue worth fixing.
    While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It doesn't affect functionality.
    The comment should be deleted as it addresses a trivial formatting issue that would be better handled by automated tools.
35. engine/baml-lib/baml/tests/validation_files/expr/expr_list.baml:7
  • Draft comment:
    The file currently does not end with a newline, which is a minor typographical style issue. Please add a newline at the end of the file.
  • 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%
    Missing newlines at end of files are typically caught by linters and formatters. This is a very minor style issue that doesn't affect functionality. The comment is about something that changed (new file), but it's too trivial to warrant a PR comment. Most IDEs and build systems would automatically fix this.
    Missing newlines can cause issues with some tools and are considered good practice. The comment is technically correct.
    While technically correct, this is exactly the kind of minor issue that should be handled by automated tools rather than PR comments. It's too trivial to warrant human attention.
    Delete this comment as it's too minor and would typically be handled by automated tooling.
36. engine/baml-lib/baml/tests/validation_files/expr/unknown_name.baml:12
  • Draft comment:
    Typographical inconsistency: In the definition of Go3 on line 12, the parameter is written as 'x:int' instead of 'x: int'. For consistency and readability, please add a space after the colon.
  • 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%
    This is a test file specifically created to demonstrate validation errors (unknown variable and function references). The spacing inconsistency is minor and doesn't affect the test's purpose. The file appears to be intentionally crafted to test the validation system, not to demonstrate proper code style. The comments at the bottom show the expected validation errors this test is meant to produce.
    The inconsistent spacing could make the test file slightly harder to read. Also, maintaining consistent style even in test files could be considered good practice.
    While consistency is good, this is a validation test file where the focus is on demonstrating error cases, not on perfect style. The spacing issue doesn't impact the test's functionality or readability significantly.
    Delete this comment as it's a minor style issue in a test file specifically created to demonstrate validation errors, not to showcase proper code style.
37. engine/baml-lib/jinja-runtime/src/output_format/types.rs:424
  • Draft comment:
    Typo: In the comment for render_possibly_recursive_type, the word 'own't' should be corrected to 'won't'.
  • 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.
38. engine/baml-lib/jsonish/src/deserializer/semantic_streaming.rs:56
  • Draft comment:
    Typographical error: In the documentation comment above process_node, the word 'vial' should be corrected to 'via'.
  • 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.
39. engine/baml-lib/parser-database/src/walkers/mod.rs:244
  • Draft comment:
    Minor: The comment 'Walk all templates strings in the schema.' could be rephrased to 'Walk all template strings in the schema.' for better consistency with the module naming. This is a trivial typographical issue.
  • 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.
40. engine/baml-lib/schema-ast/src/ast.rs:120
  • Draft comment:
    In the implementation of std::ops::Index for TypeAliasId (lines 115-122), the expect message currently reads "expected type expression". Since this index should return a type alias assignment, it would be clearer to change the error message to "expected type alias assignment".
  • 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.
41. engine/baml-lib/schema-ast/src/ast/expr.rs:2
  • Draft comment:
    Typographical issue: Consider updating 'non-llm functions' to 'non-LLM functions' in order to maintain consistency with the common capitalization of the 'LLM' acronym.
  • 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%
    The rules state not to make purely informative comments and to avoid obvious or unimportant changes. While the capitalization suggestion is technically correct, it's a very minor documentation issue that doesn't impact code functionality or clarity. The meaning is clear either way.
    The capitalization inconsistency could potentially confuse readers about whether "llm" refers to the same concept as "LLM". Documentation standards are important for maintainability.
    While documentation consistency is good, this is too minor of an issue to warrant a PR comment. The meaning is still clear, and this kind of minor style fix can be handled during regular maintenance.
    Delete the comment as it violates our rules about not making purely informative comments and avoiding unimportant changes.
42. engine/baml-lib/schema-ast/src/parser/parse_expression.rs:239
  • Draft comment:
    Typographical error: Consider changing "panicks" to "panics" in the comment above the unescape_string function.
  • 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.
43. engine/baml-lib/schema-ast/src/parser/parse_expression.rs:277
  • Draft comment:
    Typographical error: In the comment above parse_jinja_expression, consider changing "therefor" to "therefore". Also, rephrase "should be contain" to "should contain" for better 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.
44. engine/baml-lib/schema-ast/src/parser/parse_field.rs:196
  • Draft comment:
    In the default _ arm within the parse_field_type_with_attr function, the string passed to parsing_catch_all is "field_type_with_attr!" with an unexpected exclamation mark. This appears inconsistent with similar usage (e.g., "field_type_chain") and might be a typographical error. Consider removing the exclamation mark if it isn’t intentional.
  • 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.
45. engine/baml-lib/schema-ast/src/parser/parse_schema.rs:180
  • Draft comment:
    Typo: In the panic message at line 180, 'This should never happend.' should be corrected to 'This should never happen.'
  • 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.
46. engine/baml-runtime/src/lib.rs:697
  • Draft comment:
    Typo found in comment: "requeset" should be corrected to "request".
  • 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.
47. engine/baml-schema-wasm/Cargo.toml:3
  • Draft comment:
    Typo alert: The package name on line 3 is set as 'baml-schema-build', which doesn't match the directory name 'baml-schema-wasm'. Please verify if this is intentional or if it should be updated.
  • 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.
48. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:67
  • Draft comment:
    Typographical error: Consider changing 'I dont think we need this line anymore' to 'I don't think we need this line anymore' for 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.
49. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:198
  • Draft comment:
    Typographical error: The string 'BAML_PATH_SPLTTER' appears to be misspelled. Consider correcting it to 'BAML_PATH_SPLITTER'.
  • 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.
50. engine/language_client_cffi/src/ctypes.rs:152
  • Draft comment:
    Typo: the file path string "./cffi_place_holder.baml" likely should be "./cffi_placeholder.baml" (remove the extra underscore in 'place_holder').
  • 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.
51. engine/language_client_codegen/src/go/mod.rs:169
  • Draft comment:
    Consider clarifying the panic message by replacing 'expr fns' with a more descriptive term such as 'expression functions' for better clarity and consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While the suggestion would make the message slightly clearer, panic messages are primarily for developers and "expr fns" is a common shorthand that would be well understood in this context. The change is minor and doesn't materially improve code quality. The current message is not incorrect or confusing enough to warrant a change.
    The suggestion does technically improve clarity. Maybe some developers who are new to the codebase would benefit from the more explicit wording.
    While slightly more explicit, the benefit is minimal and doesn't justify the overhead of making the change. The current message is sufficient for its purpose as an internal error message.
    Delete this comment as it suggests a minor wording change that doesn't materially improve the code quality or fix any real issues.
52. engine/language_client_codegen/src/go/mod.rs:224
  • Draft comment:
    Consider clarifying the panic message by replacing 'expr fns' with a more descriptive term such as 'expression functions' for better clarity and consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While the suggestion would make the message slightly clearer, panic messages are primarily for developers and "expr fns" is a common shorthand that would be understood by anyone working with this codebase. The change is very minor and doesn't meaningfully improve code quality or fix any actual issues. The message is already clear enough for its purpose.
    The comment does point out a potential clarity improvement. Developer-facing error messages should ideally be clear and unambiguous.
    However, the improvement is extremely minor, the existing message is already understandable, and this kind of nitpick creates unnecessary review noise.
    Delete the comment as it suggests a trivial change that doesn't meaningfully improve the code.
53. engine/language_client_codegen/src/python/generate_types.rs:239
  • Draft comment:
    Typo: In the comment for add_default_value, 'Short-circuite' should be corrected to 'Short-circuit' for 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.
54. engine/language_client_codegen/src/ruby/generate_types.rs:43
  • Draft comment:
    Typo found in the comment: "Partial" on line 43. Consider correcting it to "Partial".
  • 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.
55. engine/language_client_codegen/src/typescript/mod.rs:203
  • Draft comment:
    Typographical error: 'TypscriptLlmParser' is likely misspelled; consider renaming it to 'TypescriptLlmParser' for consistency with other TypeScript identifiers.
  • 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.
56. typescript/fiddle-frontend/next.config.mjs:28
  • Draft comment:
    Typographical error: The comment on line 28 reads 'watch my locak pnpm package'. Consider changing 'locak' to 'local'.
  • Reason this comment was not posted:
    Comment was on unchanged code.
57. typescript/playground-common/src/baml_wasm_web/EventListener.tsx:80
  • Draft comment:
    In the comment on line 80, the sentence "We don't use ASTContext.provider because we should the default value of the context" seems incomplete. Consider revising it to something like "We don't use ASTContext.Provider because we use the default value of the context."
  • 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.
58. typescript/vscode-ext/packages/language-server/src/server.ts:55
  • Draft comment:
    Typographical error: The log message on line 55 reads "cant load webcrypto". Consider changing it to "can't load webcrypto" for correct punctuation.
  • 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.
59. typescript/vscode-ext/packages/language-server/src/server.ts:305
  • Draft comment:
    Typographical error: The comment on line 305 contains "mutliple". Please update it to "multiple" to correct the spelling.
  • 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.
60. typescript/vscode-ext/packages/vscode/server/linux/baml-cli:1
  • Draft comment:
    The file currently does not end with a newline character. Please consider adding a trailing newline to adhere to common file formatting conventions.
  • 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%
    Symlinks are special files that just contain a path reference. The newline convention doesn't really apply to symlinks as they are not text files in the traditional sense. The actual content of a symlink is just the target path. This comment seems to be treating the symlink as if it were a regular text file.
    Maybe there are platform-specific reasons where a newline in a symlink file could matter? Maybe this is a standard practice I'm not aware of?
    No, symlinks are handled by the filesystem at a lower level and their format is standardized. The newline character would not affect the symlink's functionality.
    This comment should be deleted as it's applying text file conventions to a symlink file where they don't apply.
61. typescript/vscode-ext/packages/vscode/src/extension.ts:69
  • Draft comment:
    Typographical error: The background color is set to '##9333ea'. It should use a single '#' (i.e., '#9333ea').
  • 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.
62. typescript/vscode-ext/packages/vscode/src/extension.ts:141
  • Draft comment:
    Typographical error: The log message 'Pushing test errorrrr' seems to have an extra 'r' in 'errorrrr'. Consider changing it to 'Pushing test error' for 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.
63. typescript/vscode-ext/packages/vscode/src/panels/WebviewPanelHost.ts:196
  • Draft comment:
    Typo detected: In the doc comment for _setWebviewMessageListener, 'recieved' should be corrected to 'received'. This is a minor, non-functional change, but fixing it will 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_QXYEL8vLKF5Wpc7V


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.

@@ -682,6 +682,7 @@ impl<'ir> ToTypeReferenceInTypeDefinition<'ir> for FieldType {
}
None => base.to_type_spec(_ir)?,
},
FieldType::Arrow(_) => todo!("Arrow types should not be used in generated type definitions"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow types in OpenAPI conversion use a todo!() placeholder. Consider handling unsupported Arrow types gracefully or documenting limitations.

Suggested change
FieldType::Arrow(_) => todo!("Arrow types should not be used in generated type definitions"),
FieldType::Arrow(_) => panic!("Arrow types are not supported in generated type definitions. Please refer to the documentation for limitations."),

Some(expr) => format!("..{}", expr.dump_str()),
None => String::new(),
};
format!("Class({} {{ {}{} }}", name, fields, spread)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the pretty-printer for Expr::ClassConstructor (dump_str method), the formatted string appears to be missing a closing parenthesis. Consider changing from:

format!("Class({} {{ {}{} }}", name, fields, spread)

to:

format!("Class({} {{ {}{} }})", name, fields, spread)

so that the output is balanced.

Suggested change
format!("Class({} {{ {}{} }}", name, fields, spread)
format!("Class({} {{ {}{} }})", name, fields, spread)

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.

👍 Looks good to me! Incremental review on 33c071e in 2 minutes and 5 seconds

More details
  • Looked at 17 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. typescript/fiddle-frontend/next.config.mjs:17
  • Draft comment:
    Confirm removal of WASM asset rule is intentional.
  • 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. typescript/fiddle-frontend/next.config.mjs:18
  • Draft comment:
    Removed explicit WASM loader. Ensure that relying solely on webpack experiments (asyncWebAssembly, syncWebAssembly) covers all use-cases for loading .wasm files.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. typescript/fiddle-frontend/next.config.mjs:22
  • Draft comment:
    Typo detected in the comment on line 22: 'watch my locak pnpm package @gloo-ai/playground-common for changes'. Consider replacing 'locak' with 'local'.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_JlNiaPwIbK8I3f1Y


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@imalsogreg imalsogreg added this pull request to the merge queue Apr 10, 2025
Merged via the queue into canary with commit 101be90 Apr 10, 2025
12 checks passed
@imalsogreg imalsogreg deleted the greg/expr-pr2 branch April 10, 2025 05:51
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.

Unify expression values and types with BAML values and types
1 participant