Auto import builds option #6064

keirlawson · 2024-01-30T23:10:04Z

Fixes scalameta/metals-feature-requests#310

tgodzik · 2024-02-03T18:18:53Z

metals/src/main/scala/scala/meta/internal/builds/BloopInstall.scala

-                // Don't spam the user with requests during rapid build changes.
-                notification.dismiss(2, TimeUnit.MINUTES)
-                Future.successful(WorkspaceLoadedStatus.Rejected)
+          if (userConfig().automaticImportBuild) {


Should we only import automatically at first or also later on?

If we want to always import automatically after changes to build files I would suggest starting it after the focused document changes, as otherwise it will be imported on each saved change, maybe missing some of the changes while the import is underway,.

Not sure I follow, you're suggesting we don't wait for the document (presumably a build.sbt) to be saved, but instead just be edited? Or are you saying we should only run when the doc is saved and has also received an update?

The problem is if someone has auto save on or saves maniacally like me 😅 In that case the import will be started at first save and later changes will be ignored until the first import finishes. This might not be an issue if someone controls it well, but would be good to avoid surprises here anyway. So instead we could do only import if the build file is not currently focused, which would indicate the user stopped editing the build definition.

An easier solution would be to do automatic import only at the very first import.

Ah I follow you now. I feel like only doing things on unfocus is probably also liable to be surprising. I think just scoping this to the initial import as you suggest is probably the most straightforward solution. I guess in practice that means reverting the changes to Indexer whilst retaining these ones to BloopInstall if I've understood correctly?

I double checked and I think you are right, otherwise it might be really hard to get right.

filipwiech · 2024-02-05T21:43:21Z

Hey @keirlawson, thank you for a great contribution, this will not only be a nice option for all the users, but it should make experience of using Metals with Helix one step better! 👍 Do you think it would make sense to make it so that this option not only performs the initial, very first build import on a fresh project, but also re-imports on each Metals start if it detects that there were any changes? It should be much simpler and safer than the already discussed on-the-fly import, but I think it would make the feature even more useful (and improve the Helix integration even more). 🙂 Asking @tgodzik too of course. 😉

keirlawson · 2024-02-05T22:08:05Z

Hey @keirlawson, thank you for a great contribution, this will not only be a nice option for all the users, but it should make experience of using Metals with Helix one step better! 👍 Do you think it would make sense to make it so that this option not only performs the initial, very first build import on a fresh project, but also re-imports on each Metals start if it detects that there were any changes? It should be much simpler and safer than the already discussed on-the-fly import, but I think it would make the feature even more useful (and improve the Helix integration even more). 🙂 Asking @tgodzik too of course. 😉

Yea I'm a Helix user as well and this was part of my motivation. Given the workflow concerns of @tgodzik though I wonder if its best to have two settings, one just for the initial import per this PR, and another for switching on auto-import in general?

tgodzik · 2024-02-06T08:43:37Z

I would rather avoid having two settings, we could have an enum instead if needed. Or ideally we would figure out the UX so that we can just have it on and off

keirlawson · 2024-02-06T18:12:31Z

Ok, have switched to an enum to control the behavior, manually tested against Helix and seems to work well

tgodzik · 2024-02-06T18:23:44Z

Ok, have switched to an enum to control the behavior, manually tested against Helix and seems to work well

Ok cool! I will merge this next week after the release if it's not a problem.

keirlawson · 2024-02-06T18:28:28Z

Ok, have switched to an enum to control the behavior, manually tested against Helix and seems to work well

Ok cool! I will merge this next week after the release if it's not a problem.

Whatever's easiest :)

filipwiech

Thank you @keirlawson for adding the extended configuration (always import), this will be really helpful! 👍

metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala

filipwiech · 2024-02-07T18:26:08Z

metals/src/main/scala/scala/meta/internal/metals/Indexer.scala

-                  tables.dismissedNotifications.ImportChanges
-                    .dismiss(2, TimeUnit.MINUTES)
-                  Future.successful(BuildChange.None)
+            if (clientConfig.autoImportBuilds == AutoImportBuildKind.All) {


About the problem that @tgodzik has mentioned previously (potentially missed import in case of multiple build changes in quick succession), maybe a future improvement would be to always save the last "intent" to import (like a checksum?) and check if it has been changed in the meantime after the currently ongoing import is finished (poor man's event queue)? 🙂

Yep I was thinking of something like that, would be interested to look at what rust-analyzer does here. That said keen to keep this PR simple, this could be a future iteration

kasiaMarek · 2024-02-12T18:28:17Z

metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala

@@ -79,6 +80,7 @@ final case class InitializationOptions(
    disableColorOutput: Option[Boolean],
    doctorVisibilityProvider: Option[Boolean],
    bspStatusBarProvider: Option[String],
+    autoImportBuild: Option[AutoImportBuildKind],


Shouldn't this be a user rather than a client setting? I think that many users no matter the client don't want to auto import, because they switch to sbt right away.

Unfortunately so far as I can see Helix, which is one of the main use cases for this I suspect, doesn't seem to have a mechanism for setting user settings

Ok digging in a bit deeper looks like helix does support didChangeConfiguration so will revert to configuring via user settings

This reverts commit 9e13b15.

…nOptions.scala Co-authored-by: Filip Wiechowski <filip.wiechowski@gmail.com>

This reverts commit 7a17c82.

filipwiech · 2024-02-15T13:13:22Z

metals/src/main/scala/scala/meta/internal/builds/BloopInstall.scala

-                notification.dismiss(2, TimeUnit.MINUTES)
-                Future.successful(WorkspaceLoadedStatus.Rejected)
+          if (
+            userConfig().automaticImportBuild == AutoImportBuildKind.Initial || userConfig().automaticImportBuild == AutoImportBuildKind.All


Sorry for the late question, but was just wondering if this will only work with Bloop, or other build servers as well (like direct BSP with sbt, mill, etc.)? Asking especially in context of the upcoming changes which would allow the user to choose the preferred build server and skip initial import with Bloop in #6121 (plus I believe there are plans to switch to sbt by default anyway). 🙂

So far as I can see looking through the code only Bloop (and possibly Bazel? https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/builds/BazelBuildTool.scala#L155-L156) send a request to the user for an initial import. @kasiaMarek @tgodzik maybe you can confirm I've got this right?

Yes and @filipwiech is probably right that in #6121 it should be changed to be consistent with what we do in Bloop and I should adjust it.

Thanks for the update, if it's going to be aligned across the board in @kasiaMarek's PR, then I think it's fine (otherwise I guess it would be nice to at least change the setting description and documentation to mention the Bloop limitation). 👍

kasiaMarek

Otherwise I'd merge this.

metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala

…tion.scala Co-authored-by: Katarzyna Marek <kasia@marek.net>

tgodzik reviewed Feb 3, 2024

View reviewed changes

keirlawson marked this pull request as ready for review February 5, 2024 20:12

keirlawson requested a review from tgodzik February 5, 2024 20:12

filipwiech reviewed Feb 7, 2024

View reviewed changes

kasiaMarek reviewed Feb 12, 2024

View reviewed changes

keirlawson and others added 7 commits February 12, 2024 21:22

Auto import builds option

53a328e

Revert indexer change

d2cd177

Change option name

83fc53f

Revert "Revert indexer change"

6d5870a

This reverts commit 9e13b15.

Switch to enum

fc8d47b

Switch to initialization option

7a17c82

Update metals/src/main/scala/scala/meta/internal/metals/Initializatio…

90304ac

…nOptions.scala Co-authored-by: Filip Wiechowski <filip.wiechowski@gmail.com>

keirlawson force-pushed the auto-import-builds branch from e90ef0c to 90304ac Compare February 12, 2024 21:22

Revert "Switch to initialization option"

7eec378

This reverts commit 7a17c82.

keirlawson requested review from kasiaMarek and filipwiech February 15, 2024 13:05

Merge branch 'main' into auto-import-builds

4518974

filipwiech reviewed Feb 15, 2024

View reviewed changes

kasiaMarek approved these changes Feb 15, 2024

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala Outdated Show resolved Hide resolved

filipwiech approved these changes Feb 15, 2024

View reviewed changes

Update metals/src/main/scala/scala/meta/internal/metals/UserConfigura…

46b2af9

…tion.scala Co-authored-by: Katarzyna Marek <kasia@marek.net>

kasiaMarek merged commit 3228b3f into scalameta:main Feb 16, 2024
25 of 26 checks passed

keirlawson deleted the auto-import-builds branch February 16, 2024 08:10

ckipp01 mentioned this pull request Feb 16, 2024

Add new init option for auto-import build scalameta/nvim-metals#650

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto import builds option #6064

Auto import builds option #6064

keirlawson commented Jan 30, 2024 •

edited

tgodzik Feb 3, 2024

keirlawson Feb 3, 2024

tgodzik Feb 3, 2024

keirlawson Feb 3, 2024

tgodzik Feb 5, 2024

filipwiech commented Feb 5, 2024 •

edited

keirlawson commented Feb 5, 2024

tgodzik commented Feb 6, 2024

keirlawson commented Feb 6, 2024

tgodzik commented Feb 6, 2024

keirlawson commented Feb 6, 2024

filipwiech left a comment

filipwiech Feb 7, 2024

keirlawson Feb 7, 2024

kasiaMarek Feb 12, 2024

keirlawson Feb 12, 2024

keirlawson Feb 12, 2024

filipwiech Feb 15, 2024 •

edited

keirlawson Feb 15, 2024

kasiaMarek Feb 15, 2024

filipwiech Feb 15, 2024

kasiaMarek left a comment

Auto import builds option #6064

Auto import builds option #6064

Conversation

keirlawson commented Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipwiech commented Feb 5, 2024 • edited

keirlawson commented Feb 5, 2024

tgodzik commented Feb 6, 2024

keirlawson commented Feb 6, 2024

tgodzik commented Feb 6, 2024

keirlawson commented Feb 6, 2024

filipwiech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipwiech Feb 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiaMarek left a comment

Choose a reason for hiding this comment

keirlawson commented Jan 30, 2024 •

edited

filipwiech commented Feb 5, 2024 •

edited

filipwiech Feb 15, 2024 •

edited