Skip to content

implement window/logMessage#3621

Merged
eed3si9n merged 1 commit intosbt:1.xfrom
eed3si9n:wip/logmessage
Nov 28, 2017
Merged

implement window/logMessage#3621
eed3si9n merged 1 commit intosbt:1.xfrom
eed3si9n:wip/logmessage

Conversation

@eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Oct 8, 2017

This sends sbt's log message as window/logMessage event to Language Server client.

This sends sbt's log message as "window/logMessage" event to LSP.
channels.foreach {
case c: ConsoleChannel =>
if (entry.channelName.isEmpty || entry.channelName == Some(c.name)) {
if (broadcastStringMessage) {
Copy link
Member

Choose a reason for hiding this comment

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

Was this for debug purposes? Is broadcastStringMessage meant to be wired up to something, rather than be a constant true?

Copy link
Member

@laughedelic laughedelic Nov 5, 2017

Choose a reason for hiding this comment

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

I think the way it was before is correct: it should take into account channel name, otherwise things get messed up..

Copy link
Member Author

Choose a reason for hiding this comment

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

This was done on purpose, so on terminal client you still see the compiler warning etc coming from the VS Code client etc.

Copy link
Member

@laughedelic laughedelic Nov 7, 2017

Choose a reason for hiding this comment

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

compiler warning etc coming from the VS Code client

I didn't get this. What do you mean? Isn't is just about messages from sbt?

@laughedelic laughedelic mentioned this pull request Nov 5, 2017
c.publishEvent(event)
// Note that language server's LogMessageParams does not hold the execid,
// so this is weaker than the StringMessage. We might want to double-send
// in case we have a better client that can utilize the knowledge.
Copy link
Member

Choose a reason for hiding this comment

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

I think execId doesn't make much sense for LSP notifications, the channelName is much more important:

  • notifications are not necessarily caused by some request, so they don't need execId (which in case of ResponseMessages is the request ID)
  • it's important that notifications go to the right channel. So if something is triggered by an LSP client, console channel shouldn't get spammed by the log messages and another way round.

Copy link
Member Author

Choose a reason for hiding this comment

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

If there are n events coming out of m commands, you need execId to distinguish them from the other.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that LSP doesn't distinguish notifications by exec-id. It does distinguish responses, but the problem here is that I see some StringEvents sent to the client with empty exec-ids, because they are sent as responses to a request that client didn't make.

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.

3 participants