Skip to content
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

fix(vscode): allow json files in baml_src #1513

Merged
merged 2 commits into from
Feb 24, 2025
Merged

fix(vscode): allow json files in baml_src #1513

merged 2 commits into from
Feb 24, 2025

Conversation

sxlijin
Copy link
Collaborator

@sxlijin sxlijin commented Feb 24, 2025

BAML, a long time ago, used to allow defining tests in JSON files, so we included them in baml_src.

Now that tests are defined exclusively in baml files, the extension implodes if there are .json files in baml_src.

This is in direct conflict with the new playground proxy setting toggle, which creates and updates .vscode/settings.json in baml_src if you have that folder open.

Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 4:19am

@sxlijin sxlijin changed the title fix: allow json files in baml_src fix(vscode): allow json files in baml_src Feb 24, 2025
@sxlijin sxlijin enabled auto-merge February 24, 2025 04:02
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to fb3c6fd in 2 minutes and 11 seconds

More details
  • Looked at 98 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. engine/baml-runtime/src/lib.rs:586
  • Draft comment:
    PR title indicates allowing JSON files, but here the valid extensions were reduced to only "baml". Verify whether removal of "json" 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 = 30% vs. threshold = 50%
    The array size mismatch is a clear issue - declaring size 2 but only having 1 element will cause a compilation error. However, we can't verify if removing "json" was intentional without seeing the PR title or description. The comment's suggestion to revert the change is speculative without that context. The array size issue is the more concrete problem.
    I may be overvaluing the array size mismatch - perhaps the PR author is aware and plans to add another extension. And I may be undervaluing the "json" removal concern - if the PR title clearly indicates JSON support should be added, this could be an important catch.
    The array size mismatch is a definite compilation error that needs to be fixed, regardless of intent. The "json" removal aspect is too speculative without seeing the PR title/description that was referenced.
    Keep a modified version of the comment that focuses on the array size mismatch compilation error, rather than speculating about the removal of "json" support.
2. typescript/vscode-ext/packages/language-server/src/file/fileUtils.ts:69
  • Draft comment:
    Comment and filter were updated to include only '.baml'; this change removes JSON files from processing. Confirm if this aligns with the PR intent.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm if the change aligns with the PR intent, which is against the rules. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
3. typescript/vscode-ext/packages/language-server/src/server.ts:161
  • Draft comment:
    Glob pattern updated to '**/*.baml', removing JSON files. This contrasts with the PR title that promises JSON support.
  • 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 is about a real change in the code - the glob pattern was indeed modified to remove JSON support. However, without seeing the PR title or description, I can't verify if this actually contradicts anything. The comment is speculative about what was promised in the PR title. Additionally, even if there was a contradiction, asking the author to explain the discrepancy is not actionable feedback.
    I could be wrong about dismissing potential inconsistencies between code changes and PR descriptions - those can sometimes indicate real issues that need to be addressed.
    While inconsistencies can be important, comments should focus on concrete code issues rather than potential discrepancies with PR metadata that we can't verify. The rules specifically say not to make speculative comments or ask authors to explain their intentions.
    The comment should be deleted as it is speculative and asks for explanation rather than suggesting a concrete code change.
4. typescript/vscode-ext/packages/package.json:18
  • Draft comment:
    Removed activation event for 'onLanguage:json' even though the PR title suggests allowing JSON files. Please reconcile this discrepancy.
  • 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%
    The comment is asking for explanation/reconciliation rather than pointing out a clear issue. It's asking about PR title consistency which is metadata, not code. Even if there is a discrepancy with the PR title, that's not necessarily a problem - the PR title could be wrong or the requirements could have changed during development.
    I could be wrong if the JSON activation is actually critical functionality that shouldn't be removed. Maybe there's important context in the PR title I'm missing.
    Even if JSON support is important, asking for explanation/reconciliation isn't actionable. If JSON support is required, the comment should directly state that instead of asking about intentions.
    Delete this comment as it's asking for explanation/reconciliation rather than pointing out a clear code issue that needs to be fixed.
5. engine/baml-runtime/src/lib.rs:584
  • Draft comment:
    PR title indicates allowing JSON files in baml_src, but the change now removes any JSON file support. Confirm if this behavior is intended.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
    The comment is asking the PR author to confirm if the behavior is intended, which violates the rule against asking for confirmation of intention. However, it does point out a potential discrepancy between the PR title and the code change, which could be useful for the author to address. The comment could be rephrased to suggest checking the alignment between the PR title and the code change without asking for confirmation.
6. typescript/vscode-ext/packages/language-server/src/file/fileUtils.ts:69
  • Draft comment:
    The file discovery condition now only checks for '.baml'. If the intent is to allow JSON files (per PR title), the condition should also include '.json'.
  • 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%
    The changes appear deliberate - both the documentation and implementation were updated to remove .json support. The comment seems to misunderstand that this was an intentional removal rather than an accidental omission. The PR title apparently mentions JSON but we should trust the code changes over speculation about PR titles.
    I could be wrong about the intention - maybe the PR title indicates this is meant to add JSON support rather than remove it. The comment author may have more context about the overall goal.
    Even if JSON support is desired, the coordinated changes to both docs and implementation strongly suggest this was an intentional removal. We should not second-guess clearly intentional changes without strong evidence.
    Delete the comment as it appears to be questioning an intentional change without strong evidence that the change was incorrect.
7. typescript/vscode-ext/packages/language-server/src/file/fileUtils.ts:83
  • Draft comment:
    convertToTextDocument now uses fileExtension.replace('.', ''). This will correctly assign 'json' for JSON files, but since gatherFiles no longer collects '.json' files, there is inconsistency.
  • 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%
    The comment points out that convertToTextDocument() will handle .json extensions even though gatherFiles() no longer collects them. However, convertToTextDocument() is a utility function that could be used independently of gatherFiles(). There's nothing inherently wrong with it supporting multiple extensions. The change to use replace() actually makes it more flexible.
    Maybe there's a broader architectural decision to completely remove JSON support from the codebase that I'm missing? The comment author might have more context about the intended direction.
    Even if there is a broader plan to remove JSON support, making convertToTextDocument() more generic isn't harmful - it's actually better software engineering to have utilities be flexible when there's no cost to doing so.
    The comment identifies an inconsistency but not a problem that requires changes. The new code is actually better - more flexible and maintainable.
8. typescript/vscode-ext/packages/language-server/src/server.ts:161
  • Draft comment:
    File watcher glob patterns have been changed from '/*.{baml, json}' to '/*.baml', which removes JSON files. Verify if JSON files should remain supported.
  • 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%
    The change appears intentional since it was made consistently across all three file watching patterns. The language server appears to be specifically for BAML files based on the imports and functionality. There's no evidence in the code that JSON files need to be watched. The comment is asking for verification without clear evidence of an issue.
    I could be wrong if JSON files are actually needed for some language server functionality that isn't visible in this file. Maybe there are JSON config files that need to be watched.
    However, the code shows this is a BAML language server with BAML-specific functionality like formatting and diagnostics. If JSON config files were needed, they would likely be handled separately rather than through these file watchers.
    The comment should be deleted as it asks for verification without evidence that JSON file watching is actually needed, and appears to be questioning an intentional change.
9. typescript/vscode-ext/packages/package.json:17
  • Draft comment:
    The 'onLanguage:json' activation event has been removed. If JSON files in baml_src should be supported per the PR title, consider reinstating this event.
  • 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%
    The comment is speculative - it's saying "if JSON files should be supported" without clear evidence that they should be. The extension appears to be primarily for .baml and Python files based on the language definitions and other configurations. Without more context about why JSON support was there in the first place or why it might be needed, this comment is not clearly actionable.
    I might be missing important context about JSON files being a critical part of baml_src functionality. The original inclusion of JSON support might have had a specific purpose.
    Even if JSON support was previously needed, the deliberate removal in this PR suggests it's no longer required. Without clear evidence that removing JSON support is a mistake, we should trust the author's intention.
    Delete the comment as it's speculative and doesn't provide clear evidence that JSON support is necessary.
10. engine/baml-runtime/src/lib.rs:586
  • Draft comment:
    The declaration for static VALID_EXTENSIONS specifies an array of size 2 but only one element ('baml') is provided. Please update the array size to 1 (or add the missing extension) to avoid a compile-time mismatch.
  • Reason this comment was not posted:
    Marked as duplicate.
11. typescript/vscode-ext/packages/language-server/src/file/fileUtils.ts:28
  • Draft comment:
    The doc comment for gatherFiles incorrectly refers to the parameter as 'uri' of type vscode.Uri, whereas the function signature uses 'rootPath' of type string. Please update the documentation to match the actual parameter name and type.
  • Reason this comment was not posted:
    Comment was on unchanged code.
12. typescript/vscode-ext/packages/language-server/src/file/fileUtils.ts:31
  • Draft comment:
    The @returns tag in the documentation for gatherFiles specifies an array of 'string[]', but the function returns an array of URI objects. Please update the @returns documentation accordingly.
  • 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.
13. typescript/vscode-ext/packages/language-server/src/server.ts:52
  • Draft comment:
    Typographical error: The log message on line 52 currently reads "cant load webcrypto". Please change it to "can't load webcrypto".
  • 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.
14. typescript/vscode-ext/packages/language-server/src/server.ts:302
  • Draft comment:
    Typographical error: The comment on line 302 contains a typo: "mutliple" should be corrected to "multiple".
  • 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.
15. typescript/vscode-ext/packages/package.json:37
  • Draft comment:
    Typo: In the description for 'baml.cliPath', consider revising 'Helps keeps generated code more in sync.' to 'Helps keep generated code more in sync.'
  • 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.
16. typescript/vscode-ext/packages/package.json:107
  • Draft comment:
    Possible typo: The 'extensions' array for the 'baml-jinja' language lists '.baml.jinja' twice. Verify if the duplicate entry is intentional or should be removed.
  • 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_Z1WTmHgm0qFCQGSw


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.

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@sxlijin sxlijin added this pull request to the merge queue Feb 24, 2025
Merged via the queue into canary with commit d81b483 Feb 24, 2025
10 of 11 checks passed
@sxlijin sxlijin deleted the sam/json-file-ext branch February 24, 2025 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant