-
Notifications
You must be signed in to change notification settings - Fork 202
add a separator to maps #2151
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
base: canary
Are you sure you want to change the base?
add a separator to maps #2151
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to e37705e in 53 seconds. Click for details.
- Reviewed
100
lines of code in2
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/ast/src/parser/datamodel.pest:105
- Draft comment:
Consider adding a brief comment explaining the purpose of the new 'separator' rule for optional colon separation in map entries. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. engine/baml-lib/ast/src/parser/datamodel.pest:106
- Draft comment:
The 'map_entry' rule now allows an optional separator. Confirm that allowing maps without a colon (e.g. {foo 1}) is intentional and won’t introduce ambiguities with other constructs. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. engine/baml-lib/ast/src/parser/parse_expression.rs:158
- Draft comment:
The new match arm for Rule::separator correctly ignores the colon token. Verify that this behavior is sufficient for handling both colon and non-colon map entries. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. engine/baml-lib/ast/src/parser/parse_expression.rs:461
- Draft comment:
New tests for map parsing without colons are a good addition. Consider adding additional tests for edge cases, such as map entries with missing values. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_JWGJeyRXcSpYALjo
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 d642cd5 in 45 seconds. Click for details.
- Reviewed
29
lines of code in2
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. engine/baml-lib/ast/src/parser/datamodel.pest:101
- Draft comment:
Good change: usingCOLON?
in the map_entry rule replaces the now-unused custom 'separator'. Consider removing the standalone 'separator' rule entirely if it's not referenced elsewhere. - 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/ast/src/parser/parse_expression.rs:158
- Draft comment:
Switching the match branch fromRule::separator
toRule::COLON
is correct and aligns with the updated grammar. The comment is clear; ensure tests cover both cases (with and without a colon). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is informative and suggests ensuring tests cover both cases, which violates the rule against asking the PR author to ensure the change is tested. It does not provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_4b7W6na8yXqI53WH
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 fdc7821 in 1 minute and 10 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/ast/src/parser/datamodel.pest:105
- Draft comment:
New empty_lines* after COLON? increases allowed whitespace between key and value. Confirm that this flexibility is consistent with similar constructs. - 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 comment is asking the author to confirm if the whitespace flexibility is consistent with similar constructs. However, looking at the full file, flexible whitespace appears to be the norm. The comment is asking for confirmation rather than pointing out a clear issue. It falls into the category of "asking author to verify/confirm" which is against our rules. The whitespace flexibility could potentially cause parsing ambiguities that I'm not seeing. There might be a good reason to be strict about whitespace in map entries specifically. While parsing edge cases are possible, the comment isn't pointing out any specific issues - it's just asking for confirmation. If there were actual parsing conflicts, they would be caught by the parser generator or tests. Delete this comment. It's asking the author to verify something rather than pointing out a clear issue, and flexible whitespace appears to be the standard pattern in this grammar.
Workflow ID: wflow_oPDZ8uKLpqVIqdE8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Add optional colon separators to map entries in BAML parser and update tests accordingly.
map_entry
indatamodel.pest
to allow optionalCOLON
aftermap_key
.parse_map_entry()
inparse_expression.rs
to handle optionalCOLON
by ignoring it.test_parse_map
,test_parse_map_without_colons
, andtest_parse_map_multiline_without_colons
inparse_expression.rs
to verify map parsing with and without colons.This description was created by
for fdc7821. You can customize this summary. It will automatically update as commits are pushed.