-
Notifications
You must be signed in to change notification settings - Fork 116
Add support for textDocument/diagnostic and workspace/diagnostic/refresh (LSP 3.17) #3557
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
base: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
- Implement standard diagnostic() method in BSLTextDocumentService - Configure DiagnosticRegistrationOptions in server capabilities - Add tests for the new diagnostic endpoint - All existing tests pass Co-authored-by: nixel2007 <[email protected]>
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLanguageServer.java
Outdated
Show resolved
Hide resolved
- Move diagnostic() logic to DiagnosticProvider - Add client capabilities check to avoid publishDiagnostics when client supports pull model - Add getDiagnosticReport() method in DiagnosticProvider - Simplify BSLTextDocumentService to delegate to DiagnosticProvider Co-authored-by: nixel2007 <[email protected]>
BSL code has inter-file dependencies where changes in one module can affect diagnostics in other modules. Co-authored-by: nixel2007 <[email protected]>
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/DiagnosticProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/DiagnosticProvider.java
Outdated
Show resolved
Hide resolved
Moved client capability check from DiagnosticProvider to BSLTextDocumentService's validate() method to maintain clear separation of responsibilities. DiagnosticProvider now focuses solely on publishing and providing diagnostics, while BSLTextDocumentService handles client capability checks. Co-authored-by: nixel2007 <[email protected]>
Renamed method for better naming consistency with LSP protocol. Co-authored-by: nixel2007 <[email protected]>
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/DiagnosticProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
Show resolved
Hide resolved
Changed from Boolean to boolean with default value of false, simplifying the accessor method. Co-authored-by: nixel2007 <[email protected]>
|
@copilot добавь ещё поддержку workspace/diagnostic/refresh в DiagnosticProvider. Если клиент поддерживает эту capability, то при получении Event на обновление LanguageServerConfiguration нужно отправлять клиенту уведомление о необходимости обновления диагностик. |
Since the field is now a primitive boolean and accessed directly, the accessor method is no longer needed. Co-authored-by: nixel2007 <[email protected]>
- Added event listener for LanguageServerInitializeRequestReceivedEvent to check if client supports workspace/diagnostic/refresh - Added event listener for LanguageServerConfigurationChangedEvent to send refresh notification when configuration changes - Client receives refreshDiagnostics() request when configuration changes and refresh is supported Co-authored-by: nixel2007 <[email protected]>
Done in a48c1c8:
|
|
@copilot тесты на diagnostic/refresh тоже нужны. |
- Added testHandleInitializeEvent to verify initialization event handling - Added testHandleInitializeEventWithoutDiagnosticsCapabilities for edge case - Added testHandleConfigurationChangedEvent to verify refresh is triggered - All tests verify that event handlers don't throw exceptions Co-authored-by: nixel2007 <[email protected]>
Done in 5506fd3:
|
|
@copilot тесты падают |
| */ | ||
| @EventListener | ||
| public void handleInitializeEvent(LanguageServerInitializeRequestReceivedEvent event) { | ||
| clientSupportsPullDiagnostics = event.getParams().getCapabilities() |
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.
@copilot получай capabilities из clientCapabilitiesHolder и обрабатывай цепочкой map, как в других примерах.
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.
Done in ea9dc69 - now using clientCapabilitiesHolder.getCapabilities() with map chain and method references:
clientSupportsPullDiagnostics = clientCapabilitiesHolder.getCapabilities()
.map(ClientCapabilities::getTextDocument)
.map(TextDocumentClientCapabilities::getDiagnostic)
.isPresent();- Added null checks in DiagnosticProvider.handleInitializeEvent for workspace and diagnostics capabilities - Added null checks in BSLTextDocumentService.handleInitializeEvent for capabilities and text document - All tests now pass Co-authored-by: nixel2007 <[email protected]>
Fixed in 3bfdeab:
|
| public void handleInitializeEvent(LanguageServerInitializeRequestReceivedEvent event) { | ||
| clientSupportsRefresh = clientCapabilitiesHolder.getCapabilities() | ||
| .map(capabilities -> capabilities.getWorkspace()) | ||
| .filter(workspace -> workspace != null) |
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.
@copilot эти проверки на null лишние, это же Optional
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.
Done in ea9dc69 - removed redundant .filter() calls. Optional's map() already handles null values properly:
clientSupportsRefresh = clientCapabilitiesHolder.getCapabilities()
.map(ClientCapabilities::getWorkspace)
.map(WorkspaceClientCapabilities::getDiagnostics)
.map(DiagnosticWorkspaceCapabilities::getRefreshSupport)
.orElse(false);- BSLTextDocumentService: Use clientCapabilitiesHolder with map chain instead of event params - DiagnosticProvider: Remove redundant null filters in Optional chain (map handles nulls) - Both use method references for cleaner code - Follows established patterns in codebase (e.g., WorkDoneProgressHelper) Co-authored-by: nixel2007 <[email protected]>
Implementation of textDocument/diagnostic and workspace/diagnostic/refresh support (LSP 3.17) ✅
Changes
1. DiagnosticProvider
getDiagnostic()method to provide pull-based diagnostics2. BSLTextDocumentService
diagnostic()method to delegate toDiagnosticProvider.getDiagnostic()LanguageServerInitializeRequestReceivedEventto detect pull diagnostics supportbooleanfield (defaultfalse) for pull diagnostics support flag, accessed directlyclientSupportsPullDiagnosticsfield invalidate()methoddiagnostic()method (follows pattern of other methods)3. BSLLanguageServer
interFileDependencies=truein DiagnosticRegistrationOptions (BSL has inter-file dependencies)4. Behavior
textDocument/diagnostic: push diagnostics are NOT published for normal operations (didOpen, didChange, didSave)workspace/diagnostic/refreshand configuration changes: client receives refresh notification to re-calculate diagnosticsResponsibility Separation
Performance Optimization
Code Quality
Testing
getDiagnostic(),computeAndPublishDiagnostics(),publishEmptyDiagnosticList()handleInitializeEvent(),handleInitializeEventWithoutDiagnosticsCapabilities(),handleConfigurationChangedEvent()Summary of Changes (4 files modified)
All tests pass ✅
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.