refactor: don't send task start/end for no-op compilations #2274
Conversation
This might be a bit opinionated and I'm not 100% sure on the reason this was done in the first place, but this changes the current behavior around reporting start and end compilations for no-ops. This still retains the compile report if it was a no-op and also still checks the diagnostics and reports them, but skips the task start/end. Originally this was added in a commit without any context... but there was a comment that said: > // When no-op, we keep reporting the start and the end of compilation for consistency However, I'm not really sure that consistency matters here. In reality this ends up creating a bunch of noise on the client side, especially when these tasks turn into LSP progress notifications that aren't useful for the user to see. You can see more context about this change in the issue reported [here](scalameta/metals#6099) and also the discussion found [here](build-server-protocol/build-server-protocol#654). It also seems that in some projects like scala-cli these are even [ignored](https://github.com/VirtusLab/scala-cli/blob/6b7a10007e4eefde717079255e0df38c027f788b/modules/build/src/main/scala/scala/build/ConsoleBloopBuildClient.scala#L109).
| | -> Msg: Compiled 'a' | ||
| | -> Data kind: compile-report | ||
| """.stripMargin | ||
| """.stripMargin // no-op so it only gives the compile-report |
There was a problem hiding this comment.
Actually, compile report is in the task finish, so we only get the compile response, no? But that should be fine as nothing really changes
There was a problem hiding this comment.
Ahh, yea sorry that's my mistake. I was switching up the names of the compile response and compile report. You're correct, it's only the compile response that is now returned.
| @@ -603,7 +590,7 @@ class BspCompileSpec( | |||
| assertSameExternalClassesDirs(fourthCompiledState, compiledState, projects) | |||
| assertNoDiff( | |||
| fourthCompiledState.lastDiagnostics(`A`), | |||
| """#4: task start 4 | |||
| """#4: task start 3 | |||
There was a problem hiding this comment.
Any idea why these changed if there was no noop compilation beforehand?
There was a problem hiding this comment.
Yea exactly, since there was a no-op in there, there is no one less task which lowers the count.
So the compile response is sent but not the compile report. Yet there is no Also it seems it would be harder to implement the same behavior in sbt, because we don't know beforehand if a compilation is going to be a no-op. |
That's a good question. I guess it'd be ideal to still know in the actual response in case clients handle them in specific ways, but then again it's the task notifications that end up being noisy, not the compile response, so maybe it doesn't matter that much.
Agreed. |
There was a problem hiding this comment.
I am okay to merge this but I also recommend to add the noOp field in the CompileResult
Sounds good. When I get a bit of time I'll outline all of this in the discussion I started in the BSP repo and re-phrase my original question and change the discussion to just aligning on how no-ops are meant to be handled. |
This might be a bit opinionated and I'm not 100% sure on the reason this was done in the first place, but this changes the current behavior around reporting start and end compilations for no-ops. This still retains the compile report if it was a no-op and also still checks the diagnostics and reports them, but skips the task start/end.
Originally this was added in a commit without any context... but there was a comment that said:
However, I'm not really sure that consistency matters here. In reality this ends up creating a bunch of noise on the client side, especially when these tasks turn into LSP progress notifications that aren't useful for the user to see. You can see more context about this change in the issue reported here and also the discussion found
here. It also seems that in some projects like scala-cli these are even ignored.