-
Notifications
You must be signed in to change notification settings - Fork 289
dont publish baml src on generate #2232
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🌿 Preview your docs: https://boundary-preview-db6881a0-a093-45e7-a62b-80fbe24c772d.docs.buildwithfern.com |
🌿 Preview your docs: https://boundary-preview-b3075214-bdf2-4df4-aa61-82f3bb8c2a03.docs.buildwithfern.com |
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 f784452 in 44 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-runtime/src/cli/generate.rs:35
- Draft comment:
Only whitespace removal here; no functional changes are introduced. Ensure that the intended behavior (not publishing baml src) is implemented as expected elsewhere. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_z24gx163CKiOWrUs
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 everything up to 33e0bea in 1 minute and 40 seconds. Click for details.
- Reviewed
91
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
5
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-runtime/src/cli/generate.rs:34
- Draft comment:
Consider defining the 'BAML_GENERATE' flag as a constant (or configuration option) to avoid duplicate string literals and potential typos in the future. - 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 constants can help prevent typos, this is a single use of a simple environment variable flag. The string is only used once in this file, and there's no evidence it's used elsewhere. Making it a constant would add complexity without much benefit. The comment is about code quality but may be overzealous given the simple, single usage. The suggestion could be valid if this flag is used in other files that we can't see. Also, constants do provide better IDE support and make refactoring easier. Without evidence of this flag being used elsewhere, the benefit of a constant is minimal. The current implementation is clear and simple for its single use. The comment should be removed as it suggests a refactor that would add complexity without clear benefit, given the single usage we can see.
2. engine/baml-runtime/src/tracingv2/publisher/publisher.rs:186
- Draft comment:
The check for 'BAML_GENERATE' correctly skips starting the publisher. Ensure that this behavior is only used for generating clients and does not inadvertently disable publisher functionality in other contexts. - 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 a specific behavior is only used in a certain context and does not affect other functionalities. This falls under the category of asking the author to double-check or ensure behavior, which is against the rules.
3. integ-tests/typescript/package.json:15
- Draft comment:
Verify that the relative path '../../engine/language_client_typescript' in the new 'build:debug' script is correct across all environments and build setups. - 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 the correctness of a relative path across all environments and build setups. This falls under the category of asking the author to double-check things, which is against the rules.
4. package.json:29
- Draft comment:
The updated 'integ-tests:ts' script now builds the debug version of the BAML client and then runs generation and tests. Confirm that this sequence aligns with your integration workflow and does not conflict with existing scripts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm that the sequence aligns with their workflow and does not conflict with existing scripts. This falls under asking the author to confirm their intention or to ensure behavior is intended, which is against the rules.
5. turbo.json:198
- Draft comment:
Review the new task configuration for 'integ-tests:ts' to ensure that its dependencies and environment variables (e.g., API keys) cover all necessary requirements for the integration tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to review and ensure that the task configuration covers all necessary requirements, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue.
Workflow ID: wflow_bFUCZ1tw6c22j1KW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-bb10d2b4-6952-4185-a317-871dcd7c6737.docs.buildwithfern.com |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Important
Prevent tracing publisher from starting during BAML generation by setting
BAML_GENERATE
environment variable and update integration test scripts and configurations.BAML_GENERATE
environment variable ingenerate_clients()
ingenerate.rs
to prevent tracing publisher from starting.start_publisher()
inpublisher.rs
checks forBAML_GENERATE
and skips publisher if set.build:debug
script topackage.json
andinteg-tests/typescript/package.json
.integ-tests:ts
script inpackage.json
to includebuild:debug
.build:debug
andinteg-tests:ts
tasks toturbo.json
with specific outputs and environment variables.This description was created by
for f784452. You can customize this summary. It will automatically update as commits are pushed.