mill icon indicating copy to clipboard operation
mill copied to clipboard

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

Open lefou opened this issue 1 year ago • 2 comments

Forgotten parenthesis on commands are a frequent cause of issue.

Here is the latest encouter:

  • https://github.com/com-lihaoyi/mill/discussions/3506#discussioncomment-10618276

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.

lefou avatar Sep 11 '24 19:09 lefou

I'd like to remove the overload

lihaoyi avatar Sep 11 '24 22:09 lihaoyi

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.

lefou avatar Sep 12 '24 09:09 lefou

Done in https://github.com/com-lihaoyi/mill/pull/4499

lihaoyi avatar Feb 08 '25 00:02 lihaoyi