-
-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bsp improve #1536
Bsp improve #1536
Conversation
Move Evaluator.Paths to EvaluatorPaths. Added EvaluatorPathsResolver.
Regarding SemanticDB: As a first step I added a new trait object myScalaModule extends ScalaModule with ScalaMetalsSupport {
def scalaVersion = ...
def semanticDbVersion = ...
...
} It's the simplest and most effective way to support Metals, if needed. We can come up with more sophisticated setups, but any clean solution is much more complex and probably involves redundant compile targets. I'm not saying we will never have them, but for now I'm mostly interested in out-of-the-box full support of (core) BSP. |
When installing, you can now select the parallel jobs to use for the BSP server, e.g. select
|
So the users would have to enable it specifically in their codebase by adding the trait? That might hard to sell to users. Maybe, with this approach it would make sense to just have instead of We could also add a way to disable it, which people could do on CI? Something long the lines of: Edit: Rather |
Yeah, you're probably right. It's the fast option to benefit right now, until we have an alternative. But I explicitly wanted to avoid some quick and dirty switch here. Most users already have a configuration trait, and it's a one-liner for them.
Yeah. I thought the same, but then, I also enabled some other scalac options and even added a filter for
Yeah. Makes sense. |
I was thinking if we could also check for existence/content of some file (roughly like |
Or make it a persistent (external) target to avoid extra files in the repo root. |
If that helps we could create it, there is always
|
I'd like to summarize: Mill has various IDE support:
As SemanticDB generation isn't cheap in terms of processing time and may have unforseen side-effects, we only want to enable it if it is in the interest of the user, which means: only enable it when the user is using Metals. Three options to handle SemanticDB: a) leaf it to the user to enable it in their build, e.g. use pros:
cons:
b) have a dedicated compile target for Metals (we could detect it in the BSP initialize request or by env var) pros:
cons:
c) automatically enable SemanticDB in case the user uses Metals pros:
cons:
How could we implement c): i) depend on external info, e.g. existence of pros:
cons:
ii) persist the fact that we were once executed from Metals pros:
cons:
|
Because it is so much clearer, separated and properly cached, I would still prefer option b) (dedicated compile target). As a side note: when we use direct IDEA support, we also compile twice (from IDE and in mill). For me this is no issue, as I almost never hit the compile button in the IDE (probably an old Eclipse habit). :p If we want to go route c) (auto-detect), than I'd prefer ii) (keep enablement in an persistent target). |
b) and c) are both fine from Metals perspective. c) might be nicer for people using CLI a lot, but whatever seems better suited for Mill should be more important here. |
Reading back through all this again, I just stumbled on this. In Metals we now auto-generate this if a user is in a Mill workspace that supports BSP. We just use the default for the # of jobs. Do you think that's fine? And if a user does want more control over it they just create it manually? |
Parallel job processing is rather stable and thus safe to use. Our reworked BSP is now usable on the happy path, but if something goes wrong, we have still rough edges (for example when we have errors in |
Reworked and improved BSP Support.
Fix #1035 - TaskFinish is never sent
Fix #986 - Support
workspace/reload
Fix #1497 - compilation while BSP synchronization
Fix #1034 - BSP TaskProgress is not implemented correctly