atom / atom-languageclient

Language Server Protocol support for Atom (the basis of Atom-IDE)
https://ide.atom.io/
MIT License
389 stars 78 forks source link

Logging server stderr #157

Closed alexheretic closed 6 years ago

alexheretic commented 6 years ago

I noticed you've disabled this. This would be very useful for Rls debugging, and I imagine any language server. I've also seen better results logging stderr lines, rather than chunks. Would you accept a PR to enable this functionality? I think it should at least be opt-in by clients.

damieng commented 6 years ago

Was considering this just yesterday in response to some comments.

Would accept a PR. I think we should probably wire up stderr to a method that takes a line of output and leaves it up to the ide-x package author to decide what to do with. They could either display messages, filter them and put the results in the debug log etc.

alexheretic commented 6 years ago

I guess that since the ide-* actually supplies the process, it's easy enough for it to handle it's own process's stderr. However, even easier if atom-languageclient just automatically logs.

I'm not sure if there's an advantage to sending the stderr to a method, if the concrete class can just mess with the stderr directly anyway.

How about one of:

damieng commented 6 years ago

I don't think we automatically want to send stderr to the debug lsp logs. Something going wrong with stderr could mean a user needs to know - they won't have this switched on and even if they did they wouldn't necessarily know it had happened.

I imagine there are a bunch of stderr messages that should alert the users but they are going to be ide-x specific as to which ones (if any).

alexheretic commented 6 years ago

Ah you're talking about actual error notifications, I'm more interested in getting the server stderr into the console to help debug stuff.

damieng commented 6 years ago

With the api I have in mind it would be easy to do both. All you'd need to do is add;

function handleStdErr(stdErr) {
   this.logger.debug(stdErr)
}

While other package authors could inspect stdErr to see if it's one of the conditions they need to handle/report to the user.

alexheretic commented 6 years ago

Ok I see your point.

Logging or blank default implementation?

Also probably nice to provide projectPath, and call per line of stderr. Plus 'stderr' is one word right, a weird programming word but still one word?

function handleStderr(stderr, projectPath) {
  this.logger.debug(`${this.getServerName()} stderr ${stderr}`)
}
alexheretic commented 6 years ago

I'll raise a pr then we can discuss the deets