Allowed logging of errors to be quieted. #165
Conversation
While using elixir-ls, frequently, the code that's sent to the LSP server (and then to elixir_sense) isn't always syntactically accurate, and sometimes elixir_sense will fail and spam the logs. After enough of these spammy logs, emacs throws the error into a mini-buffer to alert the user to some problem, which is not particularly helpful. Emitting an error message in these circumstances is dubious, since there's nothing truly wrong, but I can understand that it might be helpful for other uses of elixir_ls. To that end, I consolidated the logging functions into a module and gated them with an application variable so that other apps can enable or disable logging at their discretion.
|
I didn't integrate into logger because then it would be more difficult to quiet logging just for FYI, the following code defmodule Foo do
@spec^
def stuff(a, b) do
end
endwhere ^ is the cursor Honestly, I think the right answer here is to not throw exceptions and ignore unhandled forms in both |
Right now, this change is a no-op, but if and when this change elixir-lsp/elixir_sense#165 lands, this will quiet elixir_sense's logging in the prod environment, which will prevent logspam in lsp buffers when elixir_sense encounters invalid code.
|
I'm tot convinced that this is the right way to go. Obviously we cannot expect the code to deal with every possible incorrect/incomplete AST but silently ignoring it is also not going to be helpful. Error logging here helps in finding gaps in coverage - there is some really weird elixir code out there.
Isn't that something specific to emacs? vscode is happily logging errors to Output. Besides that, before logging as introduced here, elixir_sense would simply crash and let the language server emit error jsonrpc response. My speculation is that the problem here is logging to |
|
@lukaszsamson let me try to convince you of this approach I see four types of users.
The first thing to agree upon is that this type of error happens all the time while editing code. It is not an error, but an expected state where the code is syntactically incomplete for a short time. For me, it often happens two or three times every second until elixir_sense matches a case. Now let's talk about the users. The first user is representative of the vast majority of the people who consume elixir-sense. They're simply typing in code, and then having it loudly complain to them all the time. There's no reason at all for them to care about the messages and they're never going to check them. To sum up, the messages are wasteful to them. Disabling wasteful frequent messages is good for both UX and performance. As it stands right now, the server has to handle an exception, build an error, marshal it to JsonRPC and write it to stdout, while the client has to read the message from stdin, decode the JsonRPC and then display or buffer that message somewhere. All that work can be eliminated. The second user, needs to debug something in The third user, the elixir-ls developer, almost certainly has an in-progress copy of the code that's been compiled in dev mode, their logs will show, since logging is only enabled in production (elixir-lsp/elixir-ls#748). Finally, the fourth user is the one who's using elixir-sense somewhere else. Their project has logger integrated and they don't want to see elixir_sense errors in their logging. In short, it's a big win for users 1 and 4, and no change for 2 and 3. I don't think switching to logger messages would work as well, since you've recently added a backend for JsonRPC, and then there's no way to silence
Yes, this is specific to emacs (and more specifically, to
Well, yes, that's also bad. I'm not suggesting we go back to do that. I do, however, think that this file needs to handle all forms, even if most of them are a no-op. I'm not a fan of crashing and then picking up the pieces by using |
|
Thanks for detailed answer @scohen. You are probably right. I tend to look at the problem from my perspective. Even when not working on elixir-ls when I look for bugs in the tool to put them on my todo list. From the perspective of other users those are most likely irrelevant.
That would not work here. Most crashes happen in known forms with broken/incomplete AST |
|
@lukaszsamson Everyone looks at things from their perspective, it's a very human thing to do ;) |
I was thinking about the prior implementation, and I think it would mess up the line and files due to it calling a function in the Log module. Making pass through macros surrounded by an if simplifies things greatly.
Right now, this change is a no-op, but if and when this change elixir-lsp/elixir_sense#165 lands, this will quiet elixir_sense's logging in the prod environment, which will prevent logspam in lsp buffers when elixir_sense encounters invalid code.
While using elixir-ls, frequently, the code that's sent to the LSP server (and then to elixir_sense) isn't always syntactically accurate, and sometimes elixir_sense will fail and spam the logs. After enough of these spammy logs, emacs throws the error into a mini-buffer to alert the user to some problem, which is not particularly helpful.
Emitting an error message in these circumstances is dubious, since there's nothing truly wrong, but I can understand that it might be helpful for other uses of elixir_ls. To that end, I consolidated the logging functions into a module and gated them with an application variable so that other apps can enable or disable logging at their discretion.