bugfix: deduplicate auto connect for scala-cli #6259
Conversation
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
Outdated
Show resolved
Hide resolved
| otherDeleteEvents.map(toPath).foreach(onDelete) | ||
| // when metals creates `.bsp/<name>.json` files we want to ignore | ||
| // create event for those files | ||
| val ignoreJsonCreation = expectBspConfigCreation.get |
There was a problem hiding this comment.
Do we need this change? Could we just not connect to the server created if we are currently connecting already? So maybe ignore things in onBuildChangedUnbatched ?
There was a problem hiding this comment.
Could we just not connect to the server created if we are currently connecting already?
Yes and that is probably the proper fix want to have. The tricky part is that we need to be able to cancel if it gets stuck. I think we have some timeouts but I'm not sure on which parts and still the user might prefer to switch build server right away instead of waiting for the timeout. In that case we should cancel the old connection attempt. (#2563)
So maybe ignore things in onBuildChangedUnbatched ?
That file is created before we start connecting. I guess we can ignore things in onBuildChangedUnbatched from before creating json until we finish connecting. But then if in the meantime someone creates build.sbt it won't suggest connecting to sbt at all. Let me see if I can come up with a better way to simplify this.
Anyway this PR is not a proper fix it just deduplicates things for scala-cli but it doesn't resolve the underlying issue. I'd still be pro pushing a workaround because for scala-cli it's very visibly causing issues now and creating a proper fix might be a bit more complex.
There was a problem hiding this comment.
I ended up ignoring everything in onBuildChangedUnbatched until finishes connecting/reconnecting.
kasiaMarek
force-pushed
the
i6257
branch
2 times, most recently
from
360e054
to
0bdb71c
Compare
| onBuildToolsAdded(chosenBuildTool, changedBuilds) | ||
| case _ => Future.unit | ||
| ): Future[Unit] = | ||
| if (willGenerateBspConfig.get().nonEmpty) Future.unit |
There was a problem hiding this comment.
Why not just:
| if (willGenerateBspConfig.get().nonEmpty) Future.unit | |
| if (isConnecting.get()) Future.unit |
?
There was a problem hiding this comment.
I can't use isConnecting flag since it's raised after .bsp/<name>.json is created. I could raise it earlier but then I'd have to do it in many places to have it pair its name.
resolves: #6257