com-lihaoyi / mill

Mill is a fast JVM build tool that supports Java and Scala. 2-4x faster than Gradle and 4-10x faster than Maven for common workflows, Mill aims to make your project’s build process performant, maintainable, and flexible
https://mill-build.org/
MIT License
2.19k stars 347 forks source link

Evaluate if `T.command` overload accepting a `Task` can be removed #3517

Open lefou opened 2 months ago

lefou commented 2 months ago

Forgotten parenthesis on commands are a frequent cause of issue.

Here is the latest encouter:

T.command currently has two overloads, one accepting a Result (like most of the other task factories, T, T.input, T.task and so on) and one accepting a Task. Because of the second version, command definitions like the following don't do what the user thinks they do, although they seem to work.

override def publishLocal(localIvyRepo: String): Command[Unit] = T.command {
  super.publishLocal(localIvyRepo)
}

Instead of creating a new command, that depends on another command (the super-version) and returns just the result, it returns the super-command as-is.

The correct version is:

override def publishLocal(localIvyRepo: String): Command[Unit] = T.command {
  super.publishLocal(localIvyRepo)()
}
 override def publishLocal(localIvyRepo: String): Command[Unit] = T.command {
-  super.publishLocal(localIvyRepo)
+  super.publishLocal(localIvyRepo)()
 }

I wonder why we have this overload and if we should just remove it, since it causes a lot of issues. It was one of my first issues I had myself when I experimented with Mill IIRC and I helped others to identify it many many times. The rule of thumb since then is, to always use one extra pair of parenthesis compared to the definition side of the task you want to call.

This is quite obviously an issue with our API which we should fix.

lihaoyi commented 2 months ago

I'd like to remove the overload

lefou commented 2 months ago

As a first step, I deprecated all Target creators which accept a Task (#3524). This gives helpful messages in IDEs and on CLI and prepares the removal in Mill 0.13.