-
Notifications
You must be signed in to change notification settings - Fork 65
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
Include type aliases in Jinja #1286
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.
👍 Looks good to me! Reviewed everything up to 32c0a3a in 1 minute and 0 seconds
More details
- Looked at
591
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:20
- Draft comment:
The variablenon_structural_type_aliases
is correctly named to reflect its purpose of holding non-structural cycles. This aligns with the PR's intent to handle type alias cycles more effectively. - Reason this comment was not posted:
Confidence changes required:0%
The functionvalidate
incycle.rs
is responsible for checking type alias cycles. The comment on line 17 mentions that cycles are allowed only for maps and lists, which is consistent with the changes in the PR. The variablenon_structural_type_aliases
is correctly renamed to reflect its purpose of holding non-structural cycles. The functionreport_infinite_cycles
is used to report these cycles, which aligns with the PR's intent to handle type alias cycles more effectively.
2. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/template_strings.rs:21
- Draft comment:
Includingwalk_type_aliases
to add type aliases todefined_types
ensures that type aliases are considered during template string validation, aligning with the PR's intent. - Reason this comment was not posted:
Confidence changes required:0%
The functionvalidate
intemplate_strings.rs
is responsible for validating template strings. The addition ofwalk_type_aliases
to include type aliases in the validation process aligns with the PR's intent to handle type aliases more effectively. This change ensures that type aliases are considered when validating template strings, which is consistent with the PR description.
3. engine/baml-lib/jinja/src/evaluate_type/types.rs:43
- Draft comment:
TheType
enum now includesAlias
andRecursiveTypeAlias
, and theis_subtype_of
method has been updated to handle these, aligning with the PR's intent to handle type aliases more effectively. - Reason this comment was not posted:
Confidence changes required:0%
TheType
enum intypes.rs
has been extended to includeAlias
andRecursiveTypeAlias
, which aligns with the PR's intent to handle type aliases more effectively. Theis_subtype_of
method has been updated to handle these new variants, ensuring that type alias resolution is considered during type checking. This change is consistent with the PR description and improves the handling of type aliases.
4. engine/baml-lib/parser-database/src/types/mod.rs:82
- Draft comment:
Theresolve_type_aliases
function now computesrecursive_alias_cycles
using Tarjan's algorithm, ensuring type alias cycles are detected and resolved appropriately, aligning with the PR's intent. - Reason this comment was not posted:
Confidence changes required:0%
Theresolve_type_aliases
function inmod.rs
is responsible for resolving type aliases. The function now computesrecursive_alias_cycles
using Tarjan's algorithm, which aligns with the PR's intent to handle type alias cycles more effectively. This change ensures that type alias cycles are detected and resolved appropriately, consistent with the PR description.
5. engine/baml-lib/parser-database/src/walkers/mod.rs:295
- Draft comment:
Theto_jinja_type
function now correctly handlesTypeAlias
by includingAlias
andRecursiveTypeAlias
, ensuring type aliases are represented in Jinja types, aligning with the PR's intent. - Reason this comment was not posted:
Confidence changes required:0%
Theto_jinja_type
function inmod.rs
convertsFieldType
toType
. The handling ofTypeAlias
has been updated to includeAlias
andRecursiveTypeAlias
, which aligns with the PR's intent to handle type aliases more effectively. This change ensures that type aliases are correctly represented in Jinja types, consistent with the PR description.
Workflow ID: wflow_OajdfxE0FVbE1HRl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 0b4ea0b in 45 seconds
More details
- Looked at
69
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. engine/baml-lib/parser-database/src/walkers/mod.rs:152
- Draft comment:
The functionis_recursive_type_alias
is defined twice, once here and once inparser-database/src/lib.rs
. Consider defining it once and reusing it to avoid redundancy. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Without seeing lib.rs, I cannot verify if this function is actually duplicated. The comment could be correct, but it requires knowledge of another file to validate. Following our rules, if understanding requires more context from other files, we should delete the comment.
The function duplication could be a real issue that would benefit from being fixed. By removing this comment, we might miss an opportunity to improve code organization.
While the duplication might be real, we cannot verify it without seeing lib.rs. It's better to err on the side of caution than to keep a potentially incorrect comment.
Remove the comment since we cannot verify the claimed duplication without seeing the other file, and our rules state that cross-file issues should be ignored.
Workflow ID: wflow_4i3q7UuPRlYSggjP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on da038bd in 34 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. integ-tests/python/tests/test_functions.py:40
- Draft comment:
Unresolved merge conflict markers found. Please resolve the conflict in the import section. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_HmGZki3gk4FO64LE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Skipped PR review on 6c97885 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
is_subtype_of
now works with type aliases and this also allows better error reporting:Error:
Important
Add support for type aliases in Jinja templates, enhancing type checking and error reporting.
is_subtype_of
now supports type aliases, improving error reporting in Jinja templates.validate()
incycle.rs
checks for non-structural type alias cycles and reports them.validate()
intemplate_strings.rs
includes type aliases in defined types for Jinja validation.Alias
andRecursiveTypeAlias
variants toType
enum intypes.rs
.is_subtype_of()
to handle alias resolution.resolve_type_aliases()
intypes/mod.rs
resolves type aliases before cycle validation.recursive_alias_cycles
replacesstructural_recursive_alias_cycles
inrepr.rs
andmod.rs
.type_aliases_jinja.baml
to test type alias handling in Jinja templates.This description was created by for da038bd. It will automatically update as commits are pushed.