-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
Evaluate if T.command
overload accepting a Task
can be removed
#3517
Comments
I'd like to remove the overload |
lefou
added a commit
that referenced
this issue
Sep 12, 2024
lefou
added a commit
that referenced
this issue
Sep 12, 2024
`T.command` currently has two overloads, one accepting a `Result` (like most of the other task factories, `T`, `T.input`, `T.task` and so on) and one accepting a `Task`. Because of the second version, command definitions like the following don't do what the user thinks they do, although they seem to work. ```scala override def publishLocal(localIvyRepo: String): Command[Unit] = T.command { super.publishLocal(localIvyRepo) } ``` Instead of creating a new command that depends on another command (the `super`-version) and returns just the result, it returns the `super`-command as-is. The correct version is: ```scala override def publishLocal(localIvyRepo: String): Command[Unit] = T.command { super.publishLocal(localIvyRepo)() } ``` ```diff override def publishLocal(localIvyRepo: String): Command[Unit] = T.command { - super.publishLocal(localIvyRepo) + super.publishLocal(localIvyRepo)() } ``` This change deprecates all overloads accepting a `Task` in favor of accepting a `Result`. `Result`s can be easily acquired from a `Task` by calling `apply()` or short `()`. This is also the more correct way of re-using or re-defining tasks, since directly re-used tasks may result in an incorrect tasks context, esp. an incorrect source location, which is relevant when debugging misbehaving builds. See #3517 Pull request: #3524
As a first step, I deprecated all Target creators which accept a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Forgotten parenthesis on commands are a frequent cause of issue.
Here is the latest encouter:
T.command
currently has two overloads, one accepting aResult
(like most of the other task factories,T
,T.input
,T.task
and so on) and one accepting aTask
. Because of the second version, command definitions like the following don't do what the user thinks they do, although they seem to work.Instead of creating a new command, that depends on another command (the
super
-version) and returns just the result, it returns thesuper
-command as-is.The correct version is:
I wonder why we have this overload and if we should just remove it, since it causes a lot of issues. It was one of my first issues I had myself when I experimented with Mill IIRC and I helped others to identify it many many times. The rule of thumb since then is, to always use one extra pair of parenthesis compared to the definition side of the task you want to call.
This is quite obviously an issue with our API which we should fix.
The text was updated successfully, but these errors were encountered: