Ability to lift a ScalaVersion from a String #82

joan38 · 2023-09-26T13:28:32Z

Hi,

I'm helping @DavidGregory084 to maintain the mill-tpolecat and I just found out about this project and the split with sbt-tpolecat. Great idea! and thank you for doing this!

Now I feel like we could deprecate mill-tpolecat and only use this dependency in mill. Here is an example:

import $ivy.`org.typelevel::scalac-options:0.1.4`

object `my-project` extends ScalaModule {
  override def scalaVersion = "3.3.1"
  override def scalacOptions = ScalacOptions.defaultTokensForVersion(ScalaVersion.V3_3_0)
}

You can see this is very compact already and mill-tpolecat would not really add much value.

However the thing that bothers me is that now we have to edit the Scala version at multiple places.
With this PR it would look like this:

import $ivy.`org.typelevel::scalac-options:0.1.4`

object `my-project` extends ScalaModule {
  override def scalaVersion = "3.3.1"
  override def scalacOptions = ScalacOptions.defaultTokensForVersion(ScalaVersion.unsafeFromString(scalaVersion()))
}

Thanks

satorg · 2023-09-26T15:11:11Z

lib/src/main/scala/org/typelevel/scalacoptions/ScalaVersion.scala

+  def apply(version: String): ScalaVersion = {
+    val regex = raw"""(\d+)\.(\d+)\.(\d+).*""".r
+    version match {
+      case regex(major, minor, patch) => ScalaVersion(major.toLong, minor.toLong, patch.toLong)
+      case _                          => throw new IllegalArgumentException(s"Scala version $version not recognized")
+    }
+  }


Thank you for the PR!

I would probably go with more explicit fromString name rather than overloading apply.
Another couple of concerns (which I am personally not sure about):

should we validate the version numbers per se (i.e. make sure that we cannot create out of a nonexistent scala version like "100.200.300"?

should we return Option or even Either instead of throwing an exception? In that case there could be another unsafeFromString that would throw on an error.

Thanks for the comment.
Sounds like @armanbilge is agreeing?

So would that mean we could not upgrade to a newer Scala version until this dependency is supporting it? Might me ok to me. It's kind of the same with semanticdb. WDYT?

Sorry, let me clarify: I am 👍 to fromString and Option (or Either) but I think validation against a list of known versions is not worth the maintenance effort i.e. it's not buying us much and its making a lot of busywork to re-publish for every Scala release.

Yes, it was on the question side. Feel free to disregard the validation please. Perhaps, I overthought it.

I pushed an new version. Have a look.

satorg · 2023-10-01T07:28:17Z

lib/src/main/scala/org/typelevel/scalacoptions/ScalaVersion.scala

@@ -44,6 +44,18 @@ object ScalaVersion {
  val V3_3_0   = ScalaVersion(3, 3, 0)
  val V3_3_1   = ScalaVersion(3, 3, 1)

+  def fromString(version: String): Either[IllegalArgumentException, ScalaVersion] = {
+    val regex = raw"""(\d+)\.(\d+)\.(\d+).*""".r


I wonder, what is the last part .* in the regex is supposed for?
To me it looks like it's going to accept any arbitrary string after the last digit, like "1.2.3abc" – is that correct?

Another thought – perhaps it makes sense to keep the pre-compiled regex value in a private value outside of the method. To avoid getting it compiled every time upon the method call. Wdyt?

The .* after the digit was to catch stuff like -SNAPSHOT, but you are right here it's a bug and would blow on the patch.toLong. So I removed it.

I pushed a version with the regex pulled out.

but you are right here it's a bug and would blow on the patch.toLong

Wait, why is that? The three parenthetical groups in the regex are the major, minor, and patch, the other stuff would be omitted, no?

I hate to say it but I think the answer here is we need to write some tests for this stuff :) I don't see why we shouldn't support prerelease scala versions.

I added some tests and support for -RC or -M.. versions. Let me know what you think @armanbilge

joan38 · 2023-10-03T04:53:51Z

Are we good with this?

satorg

Sorry for the late response. Looks good to me, thank you!

joan38 · 2023-10-06T21:06:50Z

Do we need someone to merge the PR?

joan38 · 2023-10-19T19:57:58Z

@satorg @armanbilge @BalmungSan How can we move forward on this?

TonioGela · 2023-10-31T20:12:19Z

lib/src/main/scala/org/typelevel/scalacoptions/ScalaVersion.scala

@@ -44,6 +44,17 @@ object ScalaVersion {
  val V3_3_0   = ScalaVersion(3, 3, 0)
  val V3_3_1   = ScalaVersion(3, 3, 1)

+  private val versionRegex = raw"""(\d+)\.(\d+)\.(\d+)""".r


[Yak Shaving Alert] Can I suggest something like this? I think we will do another PR before either Scala 4 will come out or any minor/hotfix of any existing scala version will pass number 99.

Suggested change

private val versionRegex = raw"""(\d+)\.(\d+)\.(\d+)""".r

private val versionRegex = raw"""(2|3)\.(\d\d?)\.(\d\d?)""".r

armanbilge · 2023-10-31T20:14:27Z

How can we move forward on this?

Have you seen #82 (comment)?

SethTisue · 2023-10-31T21:13:10Z

Scala version numbers aren't just a.b.c, they're also a.b.c-Mx and a.b.c-RCx and a.b.c-bin-SHA and a.b.c-pre-SHA and a.b.c-bin-SHA-SNAPSHOT and a.b.c-pre-SHA-SNAPSHOT. Assuming only a.b.c is likely to cause problems in the Scala community build, make it difficult for project maintainers to test Scala nightlies and PR snapshots, and so on.

Might the existing methods in sbt's CrossVersion class (https://www.scala-sbt.org/1.x/api/sbt/librarymanagement/CrossVersion$.html) be usable here?

If you decide to go forward with adding your own version number parsing here, I'd suggest at minimum supporting both a.b.c and a.b.c-<arbitrary stuff>).

TonioGela · 2023-10-31T21:27:45Z

Might the existing methods in sbt's CrossVersion class (https://www.scala-sbt.org/1.x/api/sbt/librarymanagement/CrossVersion$.html) be usable here?

This could be a good idea indeed.

TonioGela · 2023-11-07T10:22:02Z

@joan38 have you tried the Seth's suggestion of leveraging sbt's CrossVersion?

armanbilge · 2023-11-07T12:31:06Z

lib/src/test/scala/org/typelevel/scalacoptions/ScalaVersionSuite.scala

+    }
+  }
+
+  property("fromString parse an RC version") {


Can we also add tests for versions like:

3.4.0-RC1-bin-20231106-f61026d-NIGHTLY

2.13.12-bin-80514f7

joan38 · 2023-11-07T17:43:55Z

@joan38 have you tried the Seth's suggestion of leveraging sbt's CrossVersion?

Sorry I totally not see the comments.
Do we want to have a dependency on SBT? This project is used in other build tools like mill.

TonioGela · 2023-11-07T19:00:02Z

@joan38 have you tried the Seth's suggestion of leveraging sbt's CrossVersion?

Sorry I totally not see the comments. Do we want to have a dependency on SBT? This project is used in other build tools like mill.

It makes sense indeed

joan38 · 2023-11-07T20:07:44Z

Thanks guys for all the great suggestions! Excited to use this.

satorg reviewed Sep 26, 2023

View reviewed changes

joan38 force-pushed the lift-scala-version branch 2 times, most recently from e239c58 to 2916a6d Compare October 1, 2023 07:06

satorg reviewed Oct 1, 2023

View reviewed changes

joan38 force-pushed the lift-scala-version branch 2 times, most recently from c0cf265 to 254928d Compare October 1, 2023 18:10

satorg approved these changes Oct 6, 2023

View reviewed changes

TonioGela reviewed Oct 31, 2023

View reviewed changes

joan38 force-pushed the lift-scala-version branch from 254928d to e814321 Compare November 2, 2023 21:23

armanbilge reviewed Nov 7, 2023

View reviewed changes

joan38 force-pushed the lift-scala-version branch from e814321 to a580823 Compare November 7, 2023 17:46

Ability to lift a ScalaVersion from a String

10d3bb2

joan38 force-pushed the lift-scala-version branch from a580823 to 10d3bb2 Compare November 7, 2023 17:50

armanbilge approved these changes Nov 7, 2023

View reviewed changes

TonioGela approved these changes Nov 7, 2023

View reviewed changes

TonioGela merged commit cbdd272 into typelevel:main Nov 7, 2023
9 checks passed

joan38 deleted the lift-scala-version branch March 6, 2024 04:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to lift a ScalaVersion from a String #82

Ability to lift a ScalaVersion from a String #82

joan38 commented Sep 26, 2023 •

edited

Loading

satorg Sep 26, 2023 •

edited

Loading

joan38 Sep 30, 2023

armanbilge Sep 30, 2023

satorg Oct 1, 2023

joan38 Oct 1, 2023

satorg Oct 1, 2023

joan38 Oct 1, 2023

armanbilge Oct 19, 2023

joan38 Nov 3, 2023

joan38 commented Oct 3, 2023

satorg left a comment

joan38 commented Oct 6, 2023

joan38 commented Oct 19, 2023 •

edited

Loading

TonioGela Oct 31, 2023

armanbilge commented Oct 31, 2023

SethTisue commented Oct 31, 2023 •

edited

Loading

TonioGela commented Oct 31, 2023

TonioGela commented Nov 7, 2023

armanbilge Nov 7, 2023

joan38 Nov 7, 2023

joan38 commented Nov 7, 2023 •

edited

Loading

TonioGela commented Nov 7, 2023

joan38 commented Nov 7, 2023

	private val versionRegex = raw"""(\d+)\.(\d+)\.(\d+)""".r
	private val versionRegex = raw"""(2\|3)\.(\d\d?)\.(\d\d?)""".r

Ability to lift a ScalaVersion from a String #82

Ability to lift a ScalaVersion from a String #82

Conversation

joan38 commented Sep 26, 2023 • edited Loading

satorg Sep 26, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joan38 commented Oct 3, 2023

satorg left a comment

Choose a reason for hiding this comment

joan38 commented Oct 6, 2023

joan38 commented Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

armanbilge commented Oct 31, 2023

SethTisue commented Oct 31, 2023 • edited Loading

TonioGela commented Oct 31, 2023

TonioGela commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joan38 commented Nov 7, 2023 • edited Loading

TonioGela commented Nov 7, 2023

joan38 commented Nov 7, 2023

joan38 commented Sep 26, 2023 •

edited

Loading

satorg Sep 26, 2023 •

edited

Loading

joan38 commented Oct 19, 2023 •

edited

Loading

SethTisue commented Oct 31, 2023 •

edited

Loading

joan38 commented Nov 7, 2023 •

edited

Loading