Skip to content
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

feat: add setting to choose preferred build server #6121

Merged
merged 7 commits into from
Feb 22, 2024

Conversation

kasiaMarek
Copy link
Contributor

@@ -570,6 +587,11 @@ object UserConfiguration {
case _ => AutoImportBuildKind.Off
}

val scalaCliLauncher = getStringKey("scala-cli-launcher")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because it seems to me it should be there but maybe there was some great scheme behind not adding this that I'm not aware of.

@ckipp01
Copy link
Member

ckipp01 commented Feb 19, 2024

I hope I'm not too late on this seeing that you're already pretty far, but I think that instead of providing a list of preferred build servers we should instead just provide a setting like "default BSP to build tool". (In the comment we can say "defaults to bloop") I would much rather just mark that as true to ensure that when I'm using sbt, it defaults to sbt and when using Mill it defaults to Mill. Then users don't need to think about any list or what they should include in it. I have a feeling that this would accomplish what most users want while providing a simpler UI. wdyt?

@kasiaMarek
Copy link
Contributor Author

wdyt?

The use-case I had in mind was: sbt, bloop, mill. So the user prefers sbt for sbt projects but bloop for mill projects, and it felt like it's better to make it more generic and allow the clients to deal with making it "presentable". However, I agree that default BSP to build tool is both easier and more understandable, and probably covers most real-life cases, so I'm fine with going with your suggestion.

Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

I'm wondering if we can use it also for Bazel 🤔
But we can unify it in a follow up.

Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

LGTM

@ckipp01 ckipp01 added the affects clients Use this if you are adding a new setting or making a change that will affect clients. label Feb 22, 2024
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Just a couple grammar nits

): ShowMessageRequestParams = {
val params = new ShowMessageRequestParams()
params.setMessage(
s"New $buildToolName workspace detected, would you like connect to $buildServerName build server?"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
s"New $buildToolName workspace detected, would you like connect to $buildServerName build server?"
s"New $buildToolName workspace detected, would you like connect to the $buildServerName build server?"

Nit

"default-bsp-to-build-tool",
"false",
"true",
"If used build server should default to build tool.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"If used build server should default to build tool.",
"Default to using build tool as your build server.",

Comment on lines 361 to 362
"""|If used build server should default to the one provided by the build tool
|instead of the default Bloop.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""|If used build server should default to the one provided by the build tool
|instead of the default Bloop.
"""|If your build tool can also serve as a build server, default to using it instead of Bloop.

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

🚀

ckipp01 added a commit to ckipp01/nvim-metals that referenced this pull request Feb 22, 2024
ckipp01 added a commit to ckipp01/nvim-metals that referenced this pull request Feb 22, 2024
@kasiaMarek kasiaMarek merged commit 9a859e0 into scalameta:main Feb 22, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects clients Use this if you are adding a new setting or making a change that will affect clients.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting default build server
3 participants