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

Add support for Scala 2.12.12 #1907

Merged
merged 1 commit into from Jul 14, 2020
Merged

Add support for Scala 2.12.12 #1907

merged 1 commit into from Jul 14, 2020

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Jul 13, 2020

No description provided.

kpbochenek
kpbochenek requested changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@kpbochenek kpbochenek left a comment •
edited

Choose a reason for hiding this comment

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

thanks for taking care of this!

scalafmt/scalafix fail

we also need to wait and update Ammonite for 2.12.12 support.

Also I think one test failure shows expected list of elements needs to be updates.

Shouldn't also version be updated in sbt, test-release.sh and test-pom.xml? just grepped where 2.12.11 was used.

tgodzik
tgodzik commented Jul 13, 2020
View reviewed changes
.github/workflows/ci.yml Outdated
@@ -99,7 +99,7 @@ jobs:
name: Scala cross tests
os: ubuntu-latest
- type: scalafix
command: sbt scalafixCheck docs/docusaurusCreateSite
command: sbt ++2.12.11 scalafixCheck docs/docusaurusCreateSite
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 will follow up with an issue to update both scalafix and ammonite when the support for both is added for Scala 2.12.12

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@tgodzik tgodzik force-pushed the update-scala-2.12.12 branch 3 times, most recently from 3b1ee05 to 94779c0 Compare July 13, 2020 16:58
@tgodzik
Copy link
Contributor Author

tgodzik commented Jul 13, 2020

Shouldn't also version be updated in sbt, test-release.sh and test-pom.xml? just grepped where 2.12.11 was used.

Updated! Thanks for reminding me! I worked around other features. Scalafix and ammonite can be updated later. This doesn't influence the actual use of Metals

@tgodzik tgodzik requested a review from kpbochenek July 13, 2020 17:01
@kpbochenek
Copy link
Contributor

@tgodzik will not ammonite scripts use default scala version which is 2.12.12 now, but it would not be supported by ammonite?

@tgodzik
Copy link
Contributor Author

tgodzik commented Jul 13, 2020 •
edited

@tgodzik will not ammonite scripts use default scala version which is 2.12.12 now, but it would not be supported by ammonite?

They use the default set by ammonite, we don't declare any default from what I've seen. I can double check.

Edit: Actually we needed to update it in couple of places. Added also TODOs to track it.

tgodzik
tgodzik commented Jul 13, 2020
View reviewed changes
metals/src/main/scala/scala/meta/internal/metals/ammonite/Ammonite.scala Outdated
@@ -149,7 +149,8 @@ final class Ammonite(
.getOrElse(
AmmVersions(
ammoniteVersion = BuildInfo.ammoniteVersion,
scalaVersion = BuildInfo.scala212
// TODO: https://github.com/scalameta/metals/issues/1908
scalaVersion = "2.12.11"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the default version for Ammonite

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

kpbochenek
kpbochenek approved these changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@kpbochenek kpbochenek left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for fast update 👍

ckipp01
ckipp01 approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@ckipp01 ckipp01 left a comment •
edited

Choose a reason for hiding this comment

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

Looks good. LGTM!

@tgodzik tgodzik force-pushed the update-scala-2.12.12 branch 4 times, most recently from 59f4e91 to 97f7747 Compare July 14, 2020 12:09
@alexarchambault
Copy link
Contributor

FYI Ammonite 2.1.4-11-307f3d8, with scala 2.12.12 support, is now on Central.

@tgodzik
Copy link
Contributor Author

tgodzik commented Jul 14, 2020

FYI Ammonite 2.1.4-11-307f3d8, with scala 2.12.12 support, is now on Central.

Thanks! That was really fast! I can now update everything normally :D

@tgodzik tgodzik merged commit 2cc43d9 into scalameta:main Jul 14, 2020
@tgodzik tgodzik deleted the update-scala-2.12.12 branch July 14, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpbochenek kpbochenek kpbochenek approved these changes

@ckipp01 ckipp01 ckipp01 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.

None yet
4 participants
@tgodzik @kpbochenek @alexarchambault @ckipp01