-
Notifications
You must be signed in to change notification settings - Fork 347
Fetch API #1916
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
Fetch API #1916
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 00894b0 in 1 minute and 35 seconds. Click for details.
- Reviewed
66lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft 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-core/src/validate/validation_pipeline/validations/expr_fns.rs:116
- Draft comment:
Consider usingis_empty()instead of comparinglen() == 0forapp.type_args, and cache the result ofapp.name.name()to avoid redundant calls. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The is_empty() suggestion is a valid Rust idiom that would make the code slightly more idiomatic. However, the caching suggestion seems premature - name() is likely just a getter and the extra variable would add complexity. The error message needs to show the function name twice for clarity. The comment combines two separate suggestions, which makes it less focused. Also, without profiling data, we don't know if caching name() would have any meaningful performance impact. The is_empty() suggestion alone would be worth keeping as it's a clear improvement following Rust conventions, but mixing it with the questionable caching suggestion weakens the comment overall. While the is_empty() suggestion has merit, the combined nature of the comment and the dubious caching suggestion make it better to skip this comment.
2. engine/baml-lib/baml/tests/validation_files/expr/builtin.baml:16
- Draft comment:
The test for missing generic type arguments is clear. Consider also adding a positive test ensuring that a valid type argument does not raise an error. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_ujFTSLukbUj5HheT
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 7054ebe in 2 minutes and 14 seconds. Click for details.
- Reviewed
80lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft 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-core/src/validate/validation_pipeline/validations/expr_typecheck.rs:260
- Draft comment:
Removal of the missing field error branch: Whenclass_walker.find_field(field_name)returns None, no error is reported. Confirm that silently skipping this case is intended; otherwise, consider restoring an explicit error message. - 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% The removed error case was for when a field name doesn't exist in the class at all. This seems like a valid error case that should be handled - if someone tries to set a field that doesn't exist, they should get an error. The later error handling only catches required fields that are missing, not invalid field names. However, the comment asks for confirmation rather than directly pointing out the issue. I could be wrong about whether this is actually a problem - maybe there's a good reason to silently ignore unknown fields, like for forward compatibility. The code also still typechecks the field value which could catch issues. While there could be reasons to ignore unknown fields, removing an error case without replacement or documentation is risky. The typecheck alone isn't enough since it would allow setting completely invalid field names. The comment identifies a real issue but phrases it as a question rather than directly stating the problem. We should rewrite it to clearly state that removing error handling for unknown field names is likely a bug.
2. fern/03-reference/baml_client/with_options.mdx:211
- Draft comment:
The Ruby async code snippet has been removed and replaced with a note indicating lack of async support. Ensure this is the desired behavior and consider linking to a roadmap or issue if future async support is planned. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the behavior is intended, which violates the rules. It also suggests linking to a roadmap or issue, which is not a direct code suggestion or request for a test. Therefore, this comment should be removed.
Workflow ID: wflow_lBl9AxfSJvaQer7A
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 29e32f1 in 2 minutes and 54 seconds. Click for details.
- Reviewed
162lines of code in4files - Skipped
0files when reviewing. - Skipped posting
8draft 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-core/src/validate/validation_pipeline/validations/expr_typecheck.rs:1
- Draft comment:
Removed the import forBuiltin; ensure built-in expressions remain accessible (viaExpr::Builtin) if needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/expr_typecheck.rs:82
- Draft comment:
Added typechecking for top-level assignments. This enhancement improves overall typecheck coverage; verify that assignments’ types are inferred as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify that the types are inferred as expected, which falls under the rule of not asking the author to confirm or ensure behavior. It doesn't provide a specific suggestion or point out a specific issue.
3. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/expr_typecheck.rs:101
- Draft comment:
The debug logging viaeprintln!has been commented out. Consider removing it before production, or using a proper logging framework. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/expr_typecheck.rs:192
- Draft comment:
The let-binding branch now safely inserts the variable type into the context using an if-let instead of unwrapping. This avoids potential panics when the type is missing. Good improvement. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what the code change does and why it's beneficial. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. According to the rules, purely informative comments should be removed.
5. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/expr_typecheck.rs:275
- Draft comment:
In the class constructor branch, ensure that error reporting for extra fields (e.g. a field not found in the class) aligns with the updated test expectations. The test now omits an error for an undefined field (like 'c'); confirm that this is intended. - 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% The comment seems to be speculating about test expectations and asking for confirmation about intended behavior. The code itself shows clear error handling for undefined fields. The comment is asking the author to verify something rather than pointing out a clear issue. This violates the rule about not asking authors to verify or confirm things. Maybe there are test changes I'm missing that would provide more context about why this verification is needed? Even if there were test changes, asking for verification rather than pointing out specific issues violates the review guidelines. The code shows clear error handling behavior. This comment should be deleted as it asks for verification rather than pointing out a clear issue, and the code already has explicit error handling for undefined fields.
6. engine/baml-lib/baml/tests/validation_files/expr/constructors.baml:6
- Draft comment:
Updated the constructor for 'MyClass' to provide a string literal for field 'b', which now correctly matches the schema. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only describes what was changed without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
7. engine/baml-lib/baml/tests/validation_files/expr/constructors_invalid.baml:21
- Draft comment:
The error annotation comments have been updated to remove the 'has no field' error for extra fields. Double-check that the typechecker’s behavior regarding undefined extra fields matches these expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to double-check the typechecker's behavior, which violates the rule against asking for confirmation or double-checking. It does not provide a specific code suggestion or ask for a specific test to be written.
8. engine/baml-lib/baml/tests/validation_files/expr/constructors_nested.baml:9
- Draft comment:
The error message in the nested constructor test has been refined to specify 'Bar.name expected type string, but found int', which improves clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only praises a change without providing any actionable feedback or suggestions. It does not align with the rules for useful comments.
Workflow ID: wflow_B4Kp64EE5Q59penu
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 ce8d88e in 1 minute and 28 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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-runtime/src/eval_expr.rs:1154
- Draft comment:
Test attribute disabled. If intentionally disabled, please add a comment explaining why; otherwise, restore #[tokio::test]. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to confirm their intention regarding a disabled test attribute. It suggests adding a comment if the test is intentionally disabled, which is a request for explanation. This violates the rule against asking the author to confirm their intention or explain their changes.
Workflow ID: wflow_EwoGJSHjZ1qY4K2y
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 4214b2b in 1 minute and 48 seconds. Click for details.
- Reviewed
464lines of code in21files - Skipped
0files when reviewing. - Skipped posting
8draft 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/language_client_codegen/src/python/generate_types.rs:42
- Draft comment:
Typo in comment: 'TypeDefinedjInBaml' should be 'TypeDefinedInBaml'. - 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/ruby/field_type.rs:10
- Draft comment:
Consider using .as_str() instead of clone() for formatting string names to reduce unnecessary allocations. - 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/typescript/mod.rs:308
- Draft comment:
The regex in add_js_suffix_to_imports is quite complex; consider adding inline comments or refactoring to improve readability 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.
4. engine/language_server/src/baml_project/mod.rs:312
- Draft comment:
Runtime caching uses DefaultHasher which is platform‐dependent. Consider using a stable hash function if file order/contents can change unexpectedly. - 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/language_client_codegen/src/python/mod.rs:240
- Draft comment:
Ensure consistency between type conversion functions in this module and those in generate_types; variation in parameters to to_type_ref and to_partial_type_ref may lead to discrepancies. - 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.
6. engine/language_client_codegen/src/ruby/generate_types.rs:139
- Draft comment:
Review the logic for handling @stream.done and @stream.not_null in PartialRubyStruct conversion to ensure proper use of T.nilable versus direct types per Sorbet conventions. - 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.
7. Multiple files:0
- Draft comment:
Replace unnecessary '.clone()' calls with borrowing (e.g. using .as_str()) where possible to avoid extra allocations. - 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.
8. Multiple files:60
- Draft comment:
Arrow types are marked as todo; ensure these cases are properly handled or emit an explicit error during code generation. - 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_r6owpQQ59LDbghds
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 27d8375 in 1 minute and 45 seconds. Click for details.
- Reviewed
25lines of code in2files - Skipped
0files when reviewing. - Skipped posting
7draft 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-schema-wasm/src/runtime_wasm/mod.rs:1617
- Draft comment:
Avoid panicking when converting JS values: several places use unwrap() after dyn_into::<js_sys::Array>(). Replace unwrap with proper error handling to safely handle unexpected types. - 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-schema-wasm/src/runtime_wasm/mod.rs:1760
- Draft comment:
Extract common helper for converting js_sys::Object environment variables to HashMap<String, String> to remove duplicate code. - 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-schema-wasm/src/runtime_wasm/mod.rs:908
- Draft comment:
Review union dummy value behavior: Only the first dummy value that exists is returned. Confirm that this is intended for union types. - 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-schema-wasm/src/runtime_wasm/mod.rs:66
- Draft comment:
Remove commented out logger initialization if no longer needed to clean up the code. - 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-schema-wasm/src/runtime_wasm/mod.rs:970
- Draft comment:
Consider reducing excessive cloning (e.g. in list_functions and test case methods) to avoid unnecessary heap allocations. - 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.
6. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:1600
- Draft comment:
Handle JS callback errors gracefully: Instead of unwrapping the call to on_partial_response in run_tests, capture and log errors to prevent panics. - 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.
7. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:1988
- Draft comment:
Standardize error conversion: Multiple functions use format!("{e:#}") with JsError::new. Consider centralizing error conversion for consistency 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.
Workflow ID: wflow_Eb2zYsk6uBOKDUqc
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 4f15177 in 1 minute and 31 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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:2894
- Draft comment:
The test assertions rely on exact multiline string matches (e.g. in render_recursive_alias_cycle). Such brittle comparisons may break with formatting tweaks. Consider using helper functions or more flexible assertions to compare essential structure rather than literal formatting. - 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:3020
- Draft comment:
Repeated use of 'IndexSet::from_iter([...].map(ToString::to_string))' could be replaced by more idiomatic collection conversion (e.g. using .iter().map(|s| s.to_string()).collect()). This could improve readability. - 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:2223
- Draft comment:
The tests are very comprehensive in covering recursive, union, and hoisted cases. Ensure that any future changes to the rendering format update these expected outputs accordingly to avoid spurious test failures. - 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_VuLyyc4kEmJuoz5b
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Introduces
fetch_valuebuilt-in function for HTTP GET requests in BAML, with parsing, type checking, and evaluation support.fetch_valuebuilt-in function inbaml-core/src/ir/builtin.rsto perform HTTP GET requests.jsonishlibrary.base_url,headers, andquery_params.parse_expr.rsandparse_expression.rsto handle generic function applications.Appstruct inast/app.rsto represent function applications with type arguments.expr_typecheck.rsto include type checking forfetch_value.eval_expr.rsto execute HTTP requests and parse responses.fiddle-proxy/server.js.validation_files/expr/builtin.bamlandconstructors_invalid.bamlto validate new functionality.field_type/mod.rsandfield_type/builder.rsto support new types and methods.This description was created by
for 4f15177. You can customize this summary. It will automatically update as commits are pushed.