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

Alignment of Default Class Implementations to Interface definitions #1604

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

snarkipus
Copy link
Contributor

Resolves #1584

Summary:

  • Added cancellation token support to various language server protocol (LSP) methods, allowing operations to be interrupted.
  • Updated method signatures across multiple LSP providers (e.g. completion, definition, document highlight) with updated documentation and optional parameters.
  • Implemented default no-op implementations for several DocumentUpdateHandler methods to reduce boilerplate in subclasses.
  • Refactored several methods to use optional parameters with default values instead of explicitly passing default values, improving code readability.

@msujew msujew added polish Some feature needs improvement LSP Language Server Protocol integration labels Jul 27, 2024
Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thank you! Most of the changes look good to me.

However, I'm not convinced of these:

  • Implemented default no-op implementations for several DocumentUpdateHandler methods to reduce boilerplate in subclasses.
  • Refactored several methods to use optional parameters with default values instead of explicitly passing default values, improving code readability.

packages/langium/src/lsp/document-update-handler.ts Outdated Show resolved Hide resolved
packages/langium/src/lsp/workspace-symbol-provider.ts Outdated Show resolved Hide resolved
packages/langium/src/lsp/workspace-symbol-provider.ts Outdated Show resolved Hide resolved
packages/langium/src/workspace/configuration.ts Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew 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, just a few minor changes, see below.

packages/langium/src/lsp/document-update-handler.ts Outdated Show resolved Hide resolved
packages/langium/src/lsp/document-update-handler.ts Outdated Show resolved Hide resolved
packages/langium/src/lsp/document-update-handler.ts Outdated Show resolved Hide resolved
packages/langium/src/workspace/configuration.ts Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew 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, thanks 👍

@msujew msujew added this to the v3.2.0 milestone Aug 14, 2024
@msujew msujew merged commit 48e67d6 into eclipse-langium:main Aug 14, 2024
4 checks passed
@snarkipus snarkipus deleted the default-class-fix branch August 14, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP Language Server Protocol integration polish Some feature needs improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default class implementations do not fully implement interfaces
3 participants