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.

Sign up for GitHub

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

Jump to bottom

improvement: auto detect project root #5576

Merged
merged 6 commits into from
Sep 22, 2023
Merged

improvement: auto detect project root #5576

merged 6 commits into from
Sep 22, 2023

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Aug 24, 2023 •
edited
Loading

Automatically detected project root with search depth of 2 levels meaning if it's:

  1. the same as workspace folder root,
  2. a direct child of workspace folder root.

connected to: #5907

@kasiaMarek kasiaMarek force-pushed the custom-project-root branch 4 times, most recently from 76a34da to e18df79 Compare August 28, 2023 13:33
@kasiaMarek kasiaMarek changed the title improvement: custom project root [WIP] improvement: custom project root Aug 28, 2023
@kasiaMarek kasiaMarek marked this pull request as ready for review August 28, 2023 17:00
@kasiaMarek kasiaMarek requested a review from tgodzik August 29, 2023 13:17
tgodzik
tgodzik requested changes Sep 4, 2023
View reviewed changes
metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/bsp/BspConfigGenerator.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/bsp/BspServers.scala Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala Outdated
@@ -2012,7 +2054,13 @@ class MetalsLspService(
&& buildTools.loadSupported.isEmpty
&& folder.hasScalaFiles
) {
scalaCli.setupIDE(folder)
languageClient
Copy link
Contributor

Choose a reason for hiding this comment

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

The change doesn't seem to be related? The automatic fallback to Scala CLI didn't seem to cause any issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit related. When you have a custom root in e.g. workspace/myProject/myProject2/, it won't be automatically detected. In that case you don't want to import as scala-cli, instead you want set the correct root in settings.

Do you think that asking the user is a worse solution? I can revert to doing that automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the automatic detection is actually pretty nice and I don't think we need to ask. If people encounter issues we can rethink it, but from what I noticed the responses were positive (or people didn't notice which is perfect)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I reverted that.

metals/src/main/scala/scala/meta/internal/builds/BuildTools.scala Outdated
if (
path.isDirectory && !path.toNIO.filename.startsWith(
"."
) && path.toNIO.filename != "project"
Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine someone having a project directory, why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in "project" we have .bloop for meta build and we don't want to detected it. Especially if something goes wrong and we have .bloop only in "project". I guess we can do it differently avoid going deeper if there is some top-level build tool (I feel like there was an argument against that approach but I can't remember now).

Copy link
Contributor

Choose a reason for hiding this comment

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

We wouldn't go to project/.bloop since we would already have .bloop in the main dir, no?

Copy link
Contributor Author

@kasiaMarek kasiaMarek Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In the happy path, yes. But if something goes wrong and .bloop in main directory isn't created, the one in project/ still will be (we even have a test for this scenario).

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 changed that do discovering bloop is correlated with the chosen build tool.

metals/src/main/scala/scala/meta/internal/builds/BuildTools.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala Outdated
@@ -313,6 +314,19 @@ object UserConfiguration {
|launcher, not available in PATH.
|""".stripMargin,
),
UserConfigurationOption(
"projects-roots",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if need the setting if this is done automatically. Seems a bit complex and people might never use workspaces, which makes this setting their roots more difficult

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above: automatically it's only for shallow levels. Yeah... This setting is a bit far from perfect. But for most users it won't be needed.

As I mentioned in the feature request, we should probably switch to workspace/configuration (from didChangeConfiguration) in the long run. It allows for scoping settings, so making settings per workspace folder will be much more intuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per workspace would be ideal here, we can introduce the setting with that change then, maybe? OTherwise we will be stuck with this much more complex setting

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 deleted the setting for now and left only auto detection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove it from UserConfiguration for now also?

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala Outdated Show resolved Hide resolved
tgodzik
tgodzik approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM! If you could just remove the added setting from UserSettings

metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala Outdated
@@ -313,6 +314,19 @@ object UserConfiguration {
|launcher, not available in PATH.
|""".stripMargin,
),
UserConfigurationOption(
"projects-roots",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove it from UserConfiguration for now also?

@tgodzik
Copy link
Contributor

tgodzik commented Sep 7, 2023

Although it looks like you need to fix the tests first 😞

@kasiaMarek kasiaMarek force-pushed the custom-project-root branch 2 times, most recently from 99d3885 to 6694a54 Compare September 8, 2023 10:44
@kasiaMarek kasiaMarek changed the title improvement: custom project root improvement: auto detect project root Sep 8, 2023
@kasiaMarek
Copy link
Contributor Author

Do you want to take another look @tgodzik? It seems there were quite a few changes with the test fixes since your approval.

tgodzik
tgodzik approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good! You might need to rebase and we can merge

@kasiaMarek kasiaMarek merged commit 4653648 into scalameta:main Sep 22, 2023
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kasiaMarek @tgodzik