Skip to content

Conversation

@xterao
Copy link
Collaborator

@xterao xterao commented Jun 3, 2025

refactors and splits the code inspection features into separate classes according to their check type:

  • Bind variable definition check
  • Loop directive type check
  • Custom function definition class setting and function call check

@xterao xterao self-assigned this Jun 3, 2025
@xterao xterao added the chore label Jun 3, 2025
@xterao xterao added this to the 0.8.1 Release milestone Jun 3, 2025
@xterao xterao linked an issue Jun 3, 2025 that may be closed by this pull request
@xterao xterao requested a review from Copilot June 3, 2025 06:21
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 refactors the SQL inspection feature by splitting its logic into three focused inspectors and wiring them through visitors and providers.

  • Introduce SqlLoopDirectiveTypeInspector and SqlFunctionCallInspector alongside existing bind-variable inspector
  • Add corresponding visitors (SqlLoopDirectiveVisitor, SqlFunctionCallVisitor) and register new providers in plugin.xml
  • Update ParameterDefinedTest to enable all three inspectors, and add HTML descriptions for the new inspections

Reviewed Changes

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

Show a summary per file
File Description
src/test/kotlin/org/domaframework/doma/intellij/inspection/sql/ParameterDefinedTest.kt Enable new loop-directive and function-call inspectors in the test
src/main/resources/inspectionDescriptions/org.domaframework.doma.intellij.loopDirectiveType.html Add HTML description for loop directive type inspection
src/main/resources/inspectionDescriptions/org.domaframework.doma.intellij.functionCall.html Add HTML description for function call inspection
src/main/resources/META-INF/plugin.xml Register SqlLoopDirectiveTypeProvider and SqlFunctionCallProvider
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/visitor/SqlLoopDirectiveVisitor.kt Add visitor implementation for loop directives
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/visitor/SqlFunctionCallVisitor.kt Add visitor implementation for function calls
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/provider/SqlLoopDirectiveTypeProvider.kt Add provider for loop directive inspection
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/provider/SqlFunctionCallProvider.kt Add provider for function call inspection
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/inspector/SqlLoopDirectiveTypeInspector.kt Add inspector for loop-directive element type checks
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/inspector/SqlFunctionCallInspector.kt Add inspector for custom function call validation
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/inspector/SqlBindVariableValidInspector.kt Update display name for bind-variable inspector
Comments suppressed due to low confidence (2)

src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/inspector/SqlBindVariableValidInspector.kt:28

  • [nitpick] The phrase "where SQL bind variables are defined" can be misread as location-focused. Consider rephrasing to "Check that SQL bind variables are defined" to clearly convey that this validation ensures declarations exist.
override fun getDisplayName(): String = "Check where SQL bind variables are defined."

src/test/kotlin/org/domaframework/doma/intellij/inspection/sql/ParameterDefinedTest.kt:48

  • [nitpick] Consider isolating loop-directive and function-call inspections into their own test classes instead of adding them to ParameterDefinedTest, to keep each test focused on a single inspection feature.
myFixture.enableInspections(

@xterao xterao requested a review from Copilot June 3, 2025 07:13
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 pull request refactors the SQL inspection features by splitting a combined inspection logic into distinct classes for bind variable checks, loop directive type checks, and custom function call inspections. Key changes include renaming and restructuring multiple inspection visitors and inspectors, creating dedicated provider classes for each inspection type, and removing previously merged DAO method variable logic.

Reviewed Changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/visitor/SqlTestDataInspectionVisitor.kt Renamed the visitor class to better reflect its purpose for test data inspection.
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/visitor/SqlLoopDirectiveTypeInspectionVisitor.kt Added a dedicated visitor to handle loop directive type checks.
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/visitor/SqlFunctionCallInspectionVisitor.kt Introduced a visitor focused on verifying function call inspections.
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/visitor/SqlBindVariableInspectionVisitor.kt Updated the bind variable inspection visitor by removing unrelated logic.
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/provider/*.kt New provider classes created to register the individual inspections.
src/main/kotlin/org/domaframework/doma/intellij/inspection/sql/inspector/*.kt Renamed and restructured inspector classes to align with the new inspection visitors.
src/main/kotlin/org/domaframework/doma/intellij/inspection/dao/visitor/*.kt Updated DAO inspection visitors to improve modularity and encapsulation.
src/main/kotlin/org/domaframework/doma/intellij/inspection/dao/inspector/*.kt Replaced the previous combined DAO method variable inspector with a new, focused implementation.

Comment on lines +48 to +50
iterator = args.minus(elements.toSet()).iterator()
while (iterator.hasNext()) {
val arg = iterator.next()
Copy link

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

Consider refactoring the iterator usage to simplify the logic; using a for-each loop to iterate over the filtered list of arguments might improve readability and maintainability.

Suggested change
iterator = args.minus(elements.toSet()).iterator()
while (iterator.hasNext()) {
val arg = iterator.next()
for (arg in args.minus(elements.toSet())) {

Copilot uses AI. Check for mistakes.
@xterao xterao merged commit 342d3f2 into main Jun 3, 2025
5 checks passed
@xterao xterao deleted the chore/split-inspection branch June 3, 2025 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split SQL Inspection into Separate Validation Passes

2 participants