feat: get baml-fmt ready for beta #1278
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 31de81f in 1 minute and 29 seconds
More details
- Looked at
603lines of code in14files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tools/build:169
- Draft comment:
Unresolved merge conflict markers detected. Please resolve the conflict by choosing the correct code block. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_6OQ42IE75keQk5to
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.
| @@ -17,7 +17,13 @@ $ pnpm add @boundaryml/baml | |||
| // biome-ignore format: autogenerated code | |||
| import { BamlRuntime, FunctionResult, BamlCtxManager, BamlSyncStream, Image, ClientRegistry, createBamlValidationError, BamlValidationError } from "@boundaryml/baml" | |||
| import { Checked, Check } from "./types" | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Unresolved merge conflict markers detected. Please resolve the conflict by choosing the correct code block.
| @@ -50,7 +50,13 @@ export default class TypeBuilder { | |||
| constructor() { | |||
| this.tb = new _TypeBuilder({ | |||
| classes: new Set([ | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Unresolved merge conflict markers detected. Please resolve the conflict by choosing the correct code block.
sxlijin
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 74f192b in 23 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/cli/src/format.rs:35
- Draft comment:
Remove the commented-out line forBamlRuntime::parse_baml_src_pathif it's not needed. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out line forBamlRuntime::parse_baml_src_pathshould be removed if it's not needed.
Workflow ID: wflow_CkZNWWBthKsNC0V4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
sxlijin
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f5d40ca in 44 seconds
More details
- Looked at
71lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/schema-ast/src/formatter/tests.rs:14
- Draft comment:
Consider reformatting theformattedstring and comparing it again to ensure idempotency of the formatter.
assert_eq!(expected.unindent().trim_end(), formatted);
let formatted = format_schema(
&formatted.unindent().trim_end(),
FormatOptions {
indent_width: 2,
fail_on_unhandled_rule: true,
},
)?;
assert_eq!(formatted.unindent().trim_end(), formatted);
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The PR author deliberately removed the double-formatting check from assert_format_eq(). They likely had a good reason for this, and the idempotency property is still being tested in other ways throughout the test file. The comment is essentially asking to revert a deliberate change.
Maybe the double-formatting check was important and removing it could let bugs slip through? Maybe there was a specific reason it was there in the first place?
The test file shows that idempotency is still being verified in multiple test cases by explicitly formatting the expected output again. The removal of the double-check from assert_format_eq() was likely to simplify the helper function while maintaining the same test coverage.
The comment should be deleted as it suggests reverting a deliberate change, and the property it's concerned about (formatter idempotency) is still being tested elsewhere in the file.
Workflow ID: wflow_xeo1hvtc00NbuIcE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bfc6041 in 15 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. fern/03-reference/baml-cli/fmt.mdx:25
- Draft comment:
Add a newline at the end of the file for better compatibility with various tools and to follow best practices. - Reason this comment was not posted:
Confidence changes required:50%
The file is missing a newline at the end, which is a best practice for text files.
Workflow ID: wflow_nYTajkfaDL8VsN4p
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 703dcc1 in 25 seconds
More details
- Looked at
138lines of code in8files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integ-tests/typescript/baml_client/sync_client.ts:20
- Draft comment:
The importsFormatterTest0,FormatterTest1,FormatterTest2, andFormatterTest3are not used in this file. Consider removing them to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The import statement forFormatterTest0,FormatterTest1,FormatterTest2, andFormatterTest3in the TypeScript filesync_client.tsseems unnecessary as these classes are not used in the file. Removing unused imports can help in reducing clutter and potential confusion.
Workflow ID: wflow_RGTBc9jbYj1Aa8g1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 179976a in 1 minute and 3 seconds
More details
- Looked at
2142lines of code in51files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. engine/language_client_python/pyproject.toml:3
- Draft comment:
The version bump to 0.71.1 is applied across multiple files. Ensure that this version change is consistent with the intended release and that all dependencies are correctly updated. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions that thebaml-cli fmtis being stabilized for beta use, focusing on class formatting. However, the code changes include updates to version numbers across multiple files, which seems unrelated to the main feature described. This could indicate a version bump for a release, but it's not explicitly mentioned in the description.
2. engine/language_client_ruby/baml.gemspec:3
- Draft comment:
The version bump to 0.71.1 is applied across multiple files. Ensure that this version change is consistent with the intended release and that all dependencies are correctly updated. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions that thebaml-cli fmtis being stabilized for beta use, focusing on class formatting. However, the code changes include updates to version numbers across multiple files, which seems unrelated to the main feature described. This could indicate a version bump for a release, but it's not explicitly mentioned in the description.
3. engine/language_client_typescript/package.json:3
- Draft comment:
The version bump to 0.71.1 is applied across multiple files. Ensure that this version change is consistent with the intended release and that all dependencies are correctly updated. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions that thebaml-cli fmtis being stabilized for beta use, focusing on class formatting. However, the code changes include updates to version numbers across multiple files, which seems unrelated to the main feature described. This could indicate a version bump for a release, but it's not explicitly mentioned in the description.
4. typescript/vscode-ext/packages/package.json:5
- Draft comment:
The version bump to 0.71.1 is applied across multiple files. Ensure that this version change is consistent with the intended release and that all dependencies are correctly updated. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions that thebaml-cli fmtis being stabilized for beta use, focusing on class formatting. However, the code changes include updates to version numbers across multiple files, which seems unrelated to the main feature described. This could indicate a version bump for a release, but it's not explicitly mentioned in the description.
Workflow ID: wflow_cGvtzv9De9gsqsXG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
baml-cli fmtis now in a state where I feel reasonably comfortable advertising it for beta use.It's relatively limited in scope: I'm pretty sure
classis the only thing it formats well;enumandfunctionstill need work.But we now have support for:
baml-format: ignoredisables the formatterImportant
Stabilizes
baml-cli fmtfor beta use, focusing on class formatting, preserving newlines and comments, and adds a directive to ignore formatting.baml-cli fmtfor beta use, focusing on formattingclassstructures.baml-format: ignoredirective to skip formatting.pretty_assertionsto1.4.1and addsregexinCargo.toml.formatter/mod.rsto handle newlines and comments better.tests.rsfor testing formatting behavior.fmt.mdxto documentbaml-cli fmtusage and limitations.This description was created by
for 179976a. It will automatically update as commits are pushed.