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

Make run command arguments Tasks #2452

Merged
merged 22 commits into from
May 1, 2023
Merged

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Apr 23, 2023

Fixes #1948. Should probably be reviewed concurrently with com-lihaoyi/mainargs#62 which it depends on

Major Changes

  1. Pull in Remove hard-coded support for mainargs.Leftover/Flag/SubParser to support alternate implementations mainargs#62, which allows MainArg's "aggregate leftover arguments" logic to apply to custom types and not be hardcoded to mainargs.Leftover

  2. Update MainArgs integration code in this repo to accomadate the changes

  3. Replace the def run(args: String*) with def run(args: Task[Args] = T.task(Args())).

Minor Changes

  1. I moved the import mill.main.TokenReaders._ into the Discover macro, so we can stop manually importing it everywhere.

  2. I moved all the aliases from import mill.define._ to import mill._. Task[_] in particular is a type I believe we should start encouraging people to use more often - e.g. annotating all command parameter types as Task[_]s - and so it should be in the default import mill._ without needing a separate import.

  3. Added a direct dependency to com.lihaoyi::pprint, since the implicit dependency in mainargs was removed in 0.5.0 due to being unnecessary

Testing

  1. All existing tests pass, at least those I've run locally so far (./mill -i -j 3 example.__.local.test, can't use CI until the mainargs PR is published). This includes unit tests that call run programmatically and integration/example tests that call run via the CLI.

  2. Added a new example test 5-module-run-task, that demonstrates and explains how to use a ScalaModule's run method to implement a task, passing in input parameters from upstream tasks and using the output (in this case generated files) in downstream tasks

Notes

  1. There is still some boilerplate around run, both defining it args: Task[Args] = T.task(Args()) and calling it programmatically, run(T.task(Args(...))). For now I haven't figured out how to reduce this, as we already "used up" our implicit conversion budget with the T.*{...} macros. Probably can be done, although it won't be quite as low-boilerplate as the original args: String* syntax. Maybe in a follow-up PR

  2. The example test 5-module-run-task is something I've wanted to be able to write for years now. Nice to finally be able to do it without having to piece together a Jvm.runSubprocess() call myself!

  3. 5-module-run-task is still a bit clunkier than I was hoping. Having to def barWorkingDir beforehand to pass it to both the bar.run and the def sources override is weird and unintuitive. Maybe we can change the def run return type to make it easier to return the T.dest folder or other metadata out of the body of def run so it can be used by downstream tasks? e.g. making def run return Command[PathRef] rather than Command[Unit]

  4. IMO we should recommend that all CLI arguments to T.commands come in the form of Task[T]s, unless there's concrete reasons otherwise (e.g. we want to dynamically change T.command implementations based on the input). That would maximise the ability to re-use commands in the body of of tasks, which is something people probably want

@lihaoyi lihaoyi changed the title Try to make run command inputs Tasks [WIP] Try to make run command inputs Tasks Apr 23, 2023
@lefou
Copy link
Member

lefou commented Apr 23, 2023

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Is there a reason to use mainargs.Leftover instead of Seq[String]?

@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 27, 2023

@lefou this was just the shortest path to verify it working end-to-end; with that mainargs PR de-hardcoding things it should be possible to swap out the type for whatever we want

We probably shouldn't go with Seq[String], because that already has meaning in mainargs as a repeatable --foo bar param, and re-using it for varargs would be confusing/conflicting. But we can make our own mill.Args(value: Seq[String]) with implicit constructors to remove boilerplate

@lihaoyi lihaoyi changed the title [WIP] Try to make run command inputs Tasks Make run command arguments Tasks Apr 27, 2023
@lihaoyi lihaoyi marked this pull request as ready for review April 27, 2023 14:21
@lihaoyi lihaoyi requested review from lefou and lolgab April 27, 2023 14:21
@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 29, 2023

@lefou this should be ready to review, looks like CI is green

@lefou lefou mentioned this pull request Apr 29, 2023
@lihaoyi lihaoyi merged commit b706a8a into com-lihaoyi:main May 1, 2023
@lefou lefou added this to the 0.11.0-M9 milestone May 1, 2023
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.

JavaModule's run command arguments should be a Task
2 participants