-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
Use Task
instead of T
when constructing targets and other tasks
#3565
Conversation
* command line and do not perform any caching. Typically used as helpers to | ||
* implement `T{...}` targets. | ||
*/ | ||
def task[T](t: Result[T]): Task[T] = macro Applicative.impl[Task, T, mill.api.Ctx] |
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.
We should deprecated this one as it overloads the term "task". Instead we should add a new def anon
with the same implementation and forward to it.
* define the tasks, while methods like `T.`[[dest]], `T.`[[log]] or | ||
* `T.`[[env]] provide the core APIs that are provided to a task implementation | ||
*/ | ||
class TargetBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] { |
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.
If we're going to abandon the name "target", we should prefer TaskBase
for this shared class.
class TargetBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] { | |
class TaskBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] { |
@lefou I also went and made the factory method names uppercase, e.g. I didn't change the type names, because a bunch of types are already just thin aliases to |
Maybe, it's enough to have the lower case creator methods in |
@lefou yep that's what I ended up doing |
Essentially a minimized binary-compatible version of #3356, renaming all the factory methods for various tasks to follow more standard naming conventions:
T {...}
->Task {...}
T.command {...}
->Task.Command {...}
T.input {...}
->Task.Input {...}
T.source {...}
->Task.Source {...}
T.sources {...}
->Task.Sources {...}
T.persistent {...}
->Task.Persistent {...}
T.task {...}
->Task.Anon {...}
The type
T[_]
remains an alias forTarget[_]
, andTask{ ... }
returns aT[_]
, to maintain binary compatibility. Not quite ideal but can probably be hand-waved away until Mill 0.13.0 when we are allowed to break binary compatibility.All the
T.*
operations have been duplicated toTask.*
by sharing them via atrait TargetBase
, except the factory methods which were copied over and upper-cased while the old version deprecated. I have updated all the code and examples to useTask
instead ofT
where relevant. The only exceptions are theimplicit def apply
s which needed to be manually copied without theimplicit
(otherwise the multiple implicits cause ambiguity).This gets us most of the user-facing benefits of #3356 without the bin-compat breakage: users no longer see an odd
T { ... }
syntax in the docs and in their build files, and now seeTask { ... }
which should be much more familiar. Although it does not allow us to do the type-hierarchy cleanups that the other PR provides, it's still worth doing so we can get it in in 0.12.0The old
T { ... }
andT.*
syntaxes should continue to work, and are exercised via the bootstrap tests as they continue to be used in Mill's own build. This PR should be source compatible to avoid migration pains, and given the prevalence ofT
everywhere we probably should just support it forever