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

Rename BAML class #1518

Merged
merged 2 commits into from
Feb 24, 2025
Merged

Rename BAML class #1518

merged 2 commits into from
Feb 24, 2025

Conversation

antoniosarosi
Copy link
Contributor

@antoniosarosi antoniosarosi commented Feb 24, 2025

Important

Add class renaming support to BAML language server with new methods for locating class definitions and handling rename requests.

  • Behavior:
    • Adds find_class_locations() to IRHelper in mod.rs to locate class definitions and references.
    • Updates WasmRuntime in mod.rs to include search_for_class_locations() and is_valid_class().
    • Implements class renaming in server.ts with textDocument/rename request, supporting only classes.
  • Misc:
    • Adds SymbolLocation struct in mod.rs for tracking symbol positions.
    • Updates getCurrentOpenedFile() in get-open-file.ts for better file handling.
    • Minor logging and error handling improvements in server.ts.

This description was created by Ellipsis for bb4826c. It will automatically update as commits are pushed.

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 5:19pm

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.

👍 Looks good to me! Reviewed everything up to fba01c5 in 2 minutes and 46 seconds

More details
  • Looked at 991 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 14 drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/src/helpers/get-open-file.ts:4
  • Draft comment:
    Good usage of optional chaining to safely retrieve the active file's URI. Consider adding an explicit return type annotation (e.g. : string | undefined) to improve clarity and type safety.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. typescript/vscode-ext/packages/vscode/src/helpers/get-open-file.ts:1
  • Draft comment:
    The import of 'URI' is not used in this file. Consider removing it to keep the code clean.
  • 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.
3. typescript/vscode-ext/packages/language-server/src/server.ts:52
  • Draft comment:
    The global assignment for crypto (polyfilling webcrypto) is applied unconditionally. Verify that this polyfill is still needed for current VS Code versions or consider a conditional check based on the target version.
  • 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.
4. typescript/vscode-ext/packages/language-server/src/server.ts:286
  • Draft comment:
    In the onDidChangeWatchedFiles handler, only the first change (params.changes[0]) is processed. Consider iterating over all file changes to properly handle multiple updates.
  • 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.
5. typescript/vscode-ext/packages/language-server/src/server.ts:749
  • Draft comment:
    In the rename request handler, renaming is not supported for enums and functions. If this limitation is intentional, add a comment documenting the current constraints and consider extending support in future iterations.
  • 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 code already has clear error messages that explain the limitations. The error messages "Enum renaming is not yet supported" and "Function renaming is not yet supported" are self-documenting. Adding a comment would be redundant. The suggestion to "consider extending support" is speculative and doesn't require immediate action.
    Perhaps having a comment would make the limitations more discoverable before hitting the runtime errors? Maybe there's value in documenting the roadmap intention?
    The error messages are clear and will be shown to users when they attempt these operations. Adding comments wouldn't make the limitations more discoverable since users would still need to look at the code. Speculative roadmap comments don't belong in the code.
    Delete the comment. The code's error messages already clearly document the limitations, and speculative suggestions about future support don't add value.
6. engine/baml-lib/baml-core/src/ir/repr.rs:411
  • Draft comment:
    Typographical error in comment: 'contraints' should be 'constraints'.
  • 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%
    While the comment correctly identifies a typo, it's a very minor issue in a code comment that doesn't affect functionality. The rules state not to make purely informative comments and to avoid obvious/unimportant issues. A typo in internal documentation is quite minor.
    The typo could cause minor confusion for future developers reading the documentation. Documentation quality does matter for maintainability.
    While documentation quality matters, this is an extremely minor typo that most readers would understand correctly from context. The cost of the PR review comment outweighs the small benefit.
    Delete this comment as it points out a very minor documentation issue that doesn't warrant a PR comment.
7. engine/baml-lib/baml-core/src/ir/repr.rs:1439
  • Draft comment:
    Typographical error in comment: 'stirng' should be 'string'.
  • 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.
8. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:66
  • Draft comment:
    Minor: Consider updating the comment at line 66 to use "don't" instead of "dont" for proper punctuation.
  • 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.
9. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:197
  • Draft comment:
    Minor: The string 'BAML_PATH_SPLTTER' at line 197 might be a typo. Consider changing it to 'BAML_PATH_SPLITTER' if that was the intended spelling.
  • 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.
10. engine/language_client_codegen/src/openapi.rs:356
  • Draft comment:
    Typo found: In the check() function, the reference string "#components/schemas/Check" is missing a leading slash. It should be "#/components/schemas/Check" to be consistent with other component references.
  • 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.
11. engine/language_client_codegen/src/python/generate_types.rs:230
  • Draft comment:
    Typo: The comment in add_default_value currently says 'Short-circuite with a None default value...'. Consider correcting 'Short-circuite' to 'Short-circuit' for clarity.
  • 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.
12. engine/language_client_codegen/src/ruby/generate_types.rs:43
  • Draft comment:
    Typographical error: The comment on line 43 reads "The Python class corresponding to Partial". It appears 'TypeDefinedjInBaml' is a typo, and likely should be 'TypeDefinedInBaml' (and perhaps the reference to 'Python' should be updated if this is for Ruby). Please fix this typo.
  • 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:54
  • Draft comment:
    Typo: Consider changing "cant" to "can't" for proper punctuation.
  • 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:305
  • Draft comment:
    Typo: 'mutliple' should be corrected to 'multiple' in the comment.
  • 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_KeSrGGhF400Qu6XM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on bb4826c in 1 minute and 59 seconds

More details
  • Looked at 34 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/ir/repr.rs:622
  • Draft comment:
    Renamed variable from 'queue' to 'stack' to better indicate DFS traversal.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, explaining a change that was made without asking for any specific action or providing a suggestion. It doesn't align with the rules for useful comments.
2. engine/baml-lib/baml-core/src/ir/repr.rs:636
  • Draft comment:
    Consistently update use of 'queue' to 'stack' in recursive branches.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. engine/baml-lib/baml-core/src/ir/repr.rs:621
  • Draft comment:
    Renamed 'queue' to 'stack' to accurately convey LIFO behavior (pop) used here.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. engine/baml-lib/baml-core/src/ir/repr.rs:636
  • Draft comment:
    Updated recursive calls to use 'stack.push' and 'stack.extend' for consistency with the renamed variable.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_YGfUYnzz2ZHW4qF7


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

pub db: &'db IntermediateRepr,
pub struct Walker<'ir, I> {
/// The IR being traversed.
pub ir: &'ir IntermediateRepr,
Copy link
Contributor

Choose a reason for hiding this comment

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

You're a good man, @antoniosarosi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man I was sooo confused I thought I was accessing the ParserDatabase @imalsogreg

@antoniosarosi antoniosarosi added this pull request to the merge queue Feb 24, 2025
Merged via the queue into canary with commit 339068a Feb 24, 2025
11 checks passed
@antoniosarosi antoniosarosi deleted the antonio/rename-baml-class branch February 24, 2025 18:42
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.

2 participants