-
-
Notifications
You must be signed in to change notification settings - Fork 323
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
Conversation
run
command inputs Tasksrun
command inputs Tasks
ebdcc45
to
1b71b89
Compare
There was a problem hiding this 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]
?
@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 |
run
command inputs Tasksrun
command arguments Tasks
@lefou this should be ready to review, looks like CI is green |
Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
Fixes #1948. Should probably be reviewed concurrently with com-lihaoyi/mainargs#62 which it depends on
Major Changes
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
Update MainArgs integration code in this repo to accomadate the changes
Replace the
def run(args: String*)
withdef run(args: Task[Args] = T.task(Args()))
.Minor Changes
I moved the
import mill.main.TokenReaders._
into theDiscover
macro, so we can stop manually importing it everywhere.I moved all the aliases from
import mill.define._
toimport 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 asTask[_]
s - and so it should be in the defaultimport mill._
without needing a separate import.Added a direct dependency to
com.lihaoyi::pprint
, since the implicit dependency inmainargs
was removed in 0.5.0 due to being unnecessaryTesting
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 callrun
programmatically and integration/example tests that callrun
via the CLI.Added a new example test
5-module-run-task
, that demonstrates and explains how to use aScalaModule
'srun
method to implement atask
, passing in input parameters from upstream tasks and using the output (in this case generated files) in downstream tasksNotes
There is still some boilerplate around
run
, both defining itargs: 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 theT.*{...}
macros. Probably can be done, although it won't be quite as low-boilerplate as the originalargs: String*
syntax. Maybe in a follow-up PRThe 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 aJvm.runSubprocess()
call myself!5-module-run-task
is still a bit clunkier than I was hoping. Having todef barWorkingDir
beforehand to pass it to both thebar.run
and thedef sources
override is weird and unintuitive. Maybe we can change thedef run
return type to make it easier to return theT.dest
folder or other metadata out of the body ofdef run
so it can be used by downstream tasks? e.g. makingdef run
returnCommand[PathRef]
rather thanCommand[Unit]
IMO we should recommend that all CLI arguments to
T.command
s come in the form ofTask[T]
s, unless there's concrete reasons otherwise (e.g. we want to dynamically changeT.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