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

Commands: Support tasks as parameters and make overriding usable #1825

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

lefou
Copy link
Member

@lefou lefou commented Apr 7, 2022

It is currently not possible to depend-on/call commands with parameters, when the parameter value comes from another task.
The typical workaround is to replace the parameter T by a Task[T], but this still required to write an additional method and delegate to it from the command. This PR allows to directly use Task[T] for any supported T, so that no additional boilerplate code is necessary.

Example:

import mill._
import mill.define._

object mod extends Module {

  def extra = T.input {
    "a"
  }

  def command1(arg: Task[String] = T.task()): Command[Unit] = T.command { 
    println(arg())
  }

  def command2(arg: String): Command[Unit] = T.command {
    command1(extra)()
  }

}

Fix #766

We also fix another issue we currently face when users tried to override commands. This never really worked out of the box, as the parameters where not statically known. With the new ability to use tasks, we can finally override command and are still be able to call super.command to reuse the overridden command result.

Here is an example:

import mill._
import mill.define._

trait Mod extends Module {
  def command1(arg: Task[String] = T.task("hello")): Command[String] = T.command {
    val text = arg()
    println(text)
    text
  }
}

object mod extends Mod {
  override def command1(arg: Task[String] = T.task("")): Command[String] = T.command {
    super.command1(T.task("overridden " + arg()))()
  }
}
$ mill showNamed mod.command1 p1
[1/1] showNamed 
[1/1] showNamed > [1/2] mod.command1.overridden.ammonite.$file.build.Mod.command1 
overridden p1
[1/1] showNamed > [2/2] mod.command1 
{
  "mod.command1": "overridden p1"
}

I think this is so useful that we should make it a best practice for commands. We even should review existing commands and refactor them in the next major release cycle (0.11).

@lefou lefou changed the title Support transparently created tasks as command parameters Support tasks as command parameters Apr 7, 2022
@lefou lefou changed the title Support tasks as command parameters Support tasks as command parameters and make overriding possible Apr 7, 2022
@lefou lefou changed the title Support tasks as command parameters and make overriding possible Commands: Support tasks as parameters and make overriding possible Apr 7, 2022
@lefou lefou changed the title Commands: Support tasks as parameters and make overriding possible Commands: Support tasks as parameters and make overriding usable Apr 7, 2022
@lefou lefou marked this pull request as ready for review April 7, 2022 18:34
@lefou lefou requested review from lihaoyi and lolgab April 7, 2022 18:37
Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

This looks really interesting.

The code change is trivial and straightforward, so not much to say there.

At first pass, this lets us call methods taking Task[T] directly from the CLI as T.command, which is a convenience we didn't have before. This might let us do without some trivial forwarders.

As a consequence, it seems we can call the T.command while making some changes to the parameters within a T.task block. This is a bit limited - you can't have code that depends on multiple Tasks in that block - but could still be useful. At a certain level of complexity, you would have to split out a helper method anyway.

Also, it seems that now T.commands are now kind of dynamic, as T.tasks already are: the exact dependencies of the command may include stuff that was passed in. We could already have conditional dependencies in the status quo, e.g. by having code that depends only on the fixed input params before calling the T.command wrapper, so I don't think this will significantly loosen any invariants

@lolgab lolgab removed their request for review April 8, 2022 09:47
@lefou
Copy link
Member Author

lefou commented Apr 8, 2022

Yeah. Most often ask question about overriding commands was in the context of run. Often, users want to hook something before the actual run occurs. Their naive attempts almost always result in execution after super.run()(), as it is a dependency. Now, they can just add their hook as arg-task to ensure, it runs before. Maybe not idiomatic or clean, it solves real world issues.

@lefou lefou merged commit 1ed23bd into com-lihaoyi:main Apr 8, 2022
@lefou lefou deleted the tasks-as-command-args branch April 8, 2022 09:56
@lefou lefou added this to the after 0.10.2 milestone Apr 8, 2022
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.

Delegate commands to take parameters computed from other tasks
2 participants