Skip to content

Conversation

@xterao
Copy link
Collaborator

@xterao xterao commented May 12, 2025

Fixed a bug that caused elements enclosed in parentheses to be recognized as a single variable name.

Modified the BNF to recognize each element within parentheses individually.

@xterao xterao added this to the 0.7.0 Release milestone May 12, 2025
@xterao xterao requested a review from Copilot May 12, 2025 08:06
@xterao xterao self-assigned this May 12, 2025
@xterao xterao added bug Something isn't working fix Bug fixes labels May 12, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where grouped conditions in SQL block comments were mistakenly recognized as bind variables by updating the BNF grammar and adjusting related parsing and test logic.

  • Updated test SQL files to verify the improved handling of function parameters in grouped conditions.
  • Modified SQLParser and symbol document test cases to reflect the changes in identification of bind variables.
  • Revised the SQL BNF grammar to individually recognize elements within parentheses.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/test/testData/src/main/resources/META-INF/doma/example/dao/EmployeeSummaryDao/bindVariableInFunctionParameters.sql Added new test block comments for grouped conditions.
src/test/testData/sql/parser/SQLParser.sql Updated conditional expressions to use explicit parentheses.
src/test/kotlin/org/domaframework/doma/intellij/document/SqlSymbolDocumentTestCase.kt Refactored field access logic to use the new accessElements extension.
src/main/kotlin/org/domaframework/doma/intellij/extension/expr/SqlElExtensions.kt Changed expression list retrieval to use getElPrimaryExprList with sorting by text offset.
src/main/java/org/domaframework/doma/intellij/Sql.bnf Adjusted grammar to include el_paren_expr in el_term_expr and modified associated pin attributes.
Comments suppressed due to low confidence (2)

src/main/java/org/domaframework/doma/intellij/Sql.bnf:139

  • Including 'el_paren_expr' in el_term_expr expands the recognized expressions; please verify that this change does not introduce parsing ambiguities or break backward compatibility.
el_term_expr ::= el_comparison_expr_group | el_arithmetic_expr_group | el_factor_expr | el_paren_expr

src/main/java/org/domaframework/doma/intellij/Sql.bnf:174

  • The removal of the '{pin=1}' attribute simplifies the rule but may affect error recovery; consider whether reintroducing a pin constraint is necessary to ensure stable parsing behavior.
private el_paren_expr ::= "(" el_expr ")"

@xterao xterao linked an issue May 12, 2025 that may be closed by this pull request
@xterao xterao force-pushed the fix/grouping-issue-in-block-comments branch from f1f985b to eccb8bd Compare May 12, 2025 08:15
@xterao xterao force-pushed the fix/grouping-issue-in-block-comments branch from eccb8bd to 9896af3 Compare May 12, 2025 08:31
@xterao xterao merged commit 9786e3a into main May 12, 2025
3 checks passed
@xterao xterao deleted the fix/grouping-issue-in-block-comments branch May 12, 2025 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working fix Bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for grouping elements in block comments

2 participants