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

fix: improve error handling for compiler diagnostics #165

Merged
merged 9 commits into from
Aug 13, 2023

Conversation

zachallaun
Copy link
Contributor

@zachallaun zachallaun commented Aug 11, 2023

Playing around with Next LS this morning and was running up against some issues compiling a project of mine. I think the previous handling of :badrpc errors was incorrect, which was hiding certain errors of causing them to bubble up in weird ways (like trying iterate over an atom).

@@ -24,7 +24,7 @@ defmodule NextLS.ElixirExtension do
end

@impl GenServer
def handle_info({:compiler, diagnostics}, state) do
def handle_info({:compiler, diagnostics}, state) when is_list(diagnostics) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

you'll need to handle an unhandled messaged, and emit a warning log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed some changes/fixes for this branch. At this point, I don't think we need to handle unexpected messages here because this should always be a list. Prior to these changes, if there was an RPC call failure, this might be like a :nodedown message or something, which was then causing a more confusing downstream error.

At this point, this guard is basically just a safety net so that, if an invalid message is sent, it's more obvious/clear in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to add: I'm fine either removing this guard or adding a catch-all for unhandled messages, but I personally think it's okay to be assertive here.

lib/next_ls/runtime.ex Outdated Show resolved Hide resolved
@zachallaun zachallaun marked this pull request as ready for review August 11, 2023 15:46
@mhanberg
Copy link
Collaborator

Looks alright, could you add a test or two?

@mhanberg
Copy link
Collaborator

just a test in the elixir_extension_test.exs is fine, would be too hard to simulate in an integration tests (the tests that use the GenLSP.Test helpers)

lib/next_ls/runtime.ex Outdated Show resolved Hide resolved
lib/next_ls/runtime.ex Outdated Show resolved Hide resolved
lib/next_ls/runtime.ex Outdated Show resolved Hide resolved
@zachallaun zachallaun force-pushed the za-patch-1 branch 2 times, most recently from edcc86a to 35c64b9 Compare August 12, 2023 17:16
mhanberg added a commit that referenced this pull request Aug 12, 2023
This should hopefully help with the tests, as they often spit out "blah
blah overlapping partitions blah blah" with regard to the clustered
nodes that are started. In test, we start many many nodes all connected
to the main node, which is the test application.

Copied some code from @zachallaun from #165

Co-authored-by: Zach Allaun <[email protected]>
@mhanberg mhanberg enabled auto-merge (squash) August 13, 2023 00:11
@mhanberg mhanberg merged commit e77cebd into elixir-tools:main Aug 13, 2023
3 checks passed
This was referenced Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants