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

Use Task instead of T when constructing targets and other tasks #3565

Merged
merged 14 commits into from
Sep 18, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Sep 17, 2024

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 for Target[_], and Task{ ... } returns a T[_], 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 to Task.* by sharing them via a trait 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 use Task instead of T where relevant. The only exceptions are the implicit def applys which needed to be manually copied without the implicit (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 see Task { ... } 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.0

The old T { ... } and T.* 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 of T everywhere we probably should just support it forever

@lihaoyi lihaoyi marked this pull request as ready for review September 17, 2024 04:18
@lihaoyi lihaoyi requested review from lolgab and lefou and removed request for lolgab September 17, 2024 04:18
* 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]
Copy link
Member

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] {
Copy link
Member

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.

Suggested change
class TargetBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] {
class TaskBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] {

@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 17, 2024

@lefou I also went and made the factory method names uppercase, e.g. Task.Command or Task.Sources, as discussed in #3338, leaving the old method names present but deprecated.

I didn't change the type names, because a bunch of types are already just thin aliases to Target already (Input, Persistent, Source, Sources), and I expect that once #3356 lands to flatten out the rest they'll basically all be aliases to Task and the issue of task type name v.s. task factory name will go away

@lefou
Copy link
Member

lefou commented Sep 17, 2024

Maybe, it's enough to have the lower case creator methods in Target (as before) and the new upper case creator methods in the new place in Task.

@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 18, 2024

@lefou yep that's what I ended up doing

@lihaoyi lihaoyi merged commit 34876b6 into com-lihaoyi:main Sep 18, 2024
24 checks passed
@lefou lefou added this to the 0.12.0 milestone Sep 18, 2024
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.

2 participants