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

Add fallback-scala-version configuration option #494

Merged

Conversation

dos65
Copy link
Member

@dos65 dos65 commented Jan 28, 2021

An addition to scalameta/metals#2417

@dos65 dos65 force-pushed the fallback_scala_version_configuration_option branch from 38b82e1 to 6126fec Compare January 28, 2021 16:36
"2.11.12",
"3.0.0-M3",
"3.0.0-M2",
"3.0.0-M1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add at least in the main Metals repo docs/contributors/releasing.md information that we should update this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to the same PR - commit

package.json Outdated Show resolved Hide resolved
@dos65 dos65 force-pushed the fallback_scala_version_configuration_option branch from caa9ffd to a00c06e Compare January 29, 2021 09:54
package.json Outdated
},
"metals.fallbackScalaVersion": {
"type": "string",
"default": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not have any default, otherwise I think empty string will be sent to the server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx, I forgot to remove it.

By the way, I remembered that there is a minor issue with this selector.
What do you think about adding an option that will be called default?

There is a logic that tries to select the highest Scala version from build targets.
It applies in case if this property is empty. But enum type doesn't allow you to choose empty so there will be no way to clean up this property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like automatic ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added automatic.

Also mentioned it in the description section. @ckipp01 could you check grammar there one more time?)

Copy link
Member

@ckipp01 ckipp01 Feb 1, 2021

Choose a reason for hiding this comment

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

Sorry, late to the party on this. I hate to say this now, but what happens if you just don't have a default, what does VS Code send? In some ways, even if it is an empty string, I'd almost rather that, and instead have Metals just filter out an empty string. Mainly because now in this meaningful list of Scala versions we have a random automatic. It's not the worst, but just treating "" as None on the Metals side instead of introducing another value makes more sense to me. It's not a hill I'll die but just my two cents.

EDIT:

The automatic value means that the Scala version for these files might be inferred as the highest supported Scala version from your project build definition

I even more so think this after reading it, mainly because this is a bit confusing. It's not the grammar, but rather an extra layer that we are adding that I don't think is necessary. Because what does "highest supported Scala version from your project build definition" really mean to someone new to Scala? Does this mean that if I have set 2.12 set for one module and 2.13 set for another, you'll choose 2.13? I'm just afraid that this may cause confusion. wdyt?

Copy link
Member Author

@dos65 dos65 Feb 1, 2021

Choose a reason for hiding this comment

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

@ckipp01
In VS Code enum option can't be manually edited. So automatic value solves the problem when you might decide to set None value back. Otherwise, you will have to manually edit .vscode/settings.json.

It might represented be as "" but I'm afraid it will more confusing than automatic or smth like that.
Unfortunately, there is no way to specify how options should be rendered in VS Code selector. Only plain values.

Does this mean that if I have set 2.12 set for one module and 2.13 set for another, you'll choose 2.13? I'm just afraid that this may cause confusion

Yes, it will select 2.13.
I don't think that is confusing because most of the users use a single Scala version per project and will never face the need to change this option.

Copy link
Member

Choose a reason for hiding this comment

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

Yea I see what you mean.

So given the following situation where I have 2 build targets, one a Scala 3 and one Scala 2 one. I create a standalone Ammonite script at the root of my project and have this set to automatic, what version of the compiler will I get for that standalone file?

Copy link
Member Author

Choose a reason for hiding this comment

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

For an Ammonite script, it will be Scala 2 because it doesn't support scala 3 yet.
For a worksheet or scala file, it will be Scala 3

@dos65 dos65 force-pushed the fallback_scala_version_configuration_option branch from a00c06e to 75d3b57 Compare February 1, 2021 12:43
package.json Outdated Show resolved Hide resolved
Co-authored-by: Chris Kipp <ckipp@pm.me>
@tgodzik tgodzik merged commit eabb9cd into scalameta:master Feb 20, 2021
kasiaMarek pushed a commit to kasiaMarek/metals-vscode that referenced this pull request Mar 29, 2023
…rn/types/node-18.7.15

chore(deps-dev): bump @types/node from 18.7.14 to 18.7.15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants