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

backtick when path segment is scala keyword #3865

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

chikei
Copy link
Contributor

@chikei chikei commented Oct 29, 2024

to allow submodule use scala keyword as name, fix #3863

I am not sure this is the way we want to match scala keyword though

package build.`import`
import mill._, scalalib._

object `package` extends RootModule with ScalaModule {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not make this a ScalaModule, since we don't care about the scala side at all. Instead, we can just add a trivial def task = Task { 123 }

Furthermore, let's exercise a few more Scala keywords, with this/package.mill, for/package.mill, if/package.mill, just to make sure it works for more than just the single import keyword

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 added few more case. One interesting point is that `package` will fail but I assume it's not due to backtick as mill complain this way:

[2758] [build.mill-57] [error] /home/Chikei/repos/mill/out/integration/feature/keyword-module/local/test.dest/sandbox/run-1/out/mill-build/generateScriptSources.dest/build_/build.mill:21:21: lazy value millDiscover overrides nothing
[2758] [build.mill-57] [error]   override lazy val millDiscover: _root_.mill.define.Discover = _root_.mill.define.Discover[this.type]
[2758] [build.mill-57] [error]                     ^
[2758] [build.mill-57] [error] one error found

So I just removed it from cases for now.

@@ -33,7 +34,8 @@ case class FileImportGraph(
object FileImportGraph {
def backtickWrap(s: String): String = s match {
case s"`$v`" => s
case _ => if (encode(s) == s) s else "`" + s + "`"
case _ => if (encode(s) == s) runtime.universe.asInstanceOf[runtime.JavaUniverse].quotedName(s)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than relying on runtime.JavaUniverse, let's instead port over the user-land implementation from Ammonite. That will make it a smoother upgrade to Scala 3 where runtime.JavaUniverse might not exist

https://github.com/com-lihaoyi/Ammonite/blob/b98bc05c0d90f43c46bafd69736ed22dd36f8734/amm/util/src/main/scala/ammonite/util/Model.scala#L127

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I just grab the alphabet keywords from ammonite since NameTransformer should handle the other case and that exists in scala 3 stdlib

@lihaoyi lihaoyi merged commit a9f828e into com-lihaoyi:main Oct 30, 2024
24 checks passed
@lefou lefou added this to the 0.12.2 milestone Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

keyword as submodule name in 0.12
3 participants