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

Unify all error handling for the server #527

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Nov 18, 2024

Closes Shopify/ruby-lsp#2886

We cannot use blanket rescue statements for the entire server because the behaviour for handling errors is different between requests and notifications.

For requests, we want to return error: message instead of the typicaly result: something. And for notifications, we cannot return anything, we can only log that an error occurred.

The approach that I took here is to create handlers that we can use both ourselves and in server add-ons, that provide appropriate error handling for requests or notifications.

I also created convenience methods for returning responses or errors.

@vinistock vinistock added the bugfix This PR fixes an existing bug label Nov 18, 2024 — with Graphite App
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock marked this pull request as ready for review November 18, 2024 19:17
@vinistock vinistock requested a review from a team as a code owner November 18, 2024 19:17
Comment on lines +2 to +6
"rubyLsp.indexing": {
"includedPatterns": [
"test/dummy/**/*.rb"
]
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to ensure that the dummy files are indexed to be able to test our own features. A recent optimization in the LSP made us fully ignore the entire test directory straight away.

@vinistock vinistock force-pushed the 11-18-unify_all_error_handling_for_the_server branch from 8b98849 to 342a2d5 Compare November 21, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server fails on boot without meaningful error message
1 participant