-
-
Notifications
You must be signed in to change notification settings - Fork 359
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 Target
type abstract to allow overriding by different concrete implementations
#2402
Conversation
Target
type abstract to allow overriding by different concrete implementationsTarget
type abstract to allow overriding by different concrete implementations
The correct-number-of-parameter-lists checks no longer work, since they used to inspect the return types of the methods, which they can no longer do since they all have the same return type. I'll see if I can find a different way to implement them |
Does this affect also |
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.
This is a great change. In my early Mill days, it took me a while to understand, that all these T
-macros return different types and that these are not compatible. In the mental model, they are all interchangeable, and this PR is just correcting it. Great!
@lefou currently, this PR does make |
@lefou maybe two questions I would like to discuss, which concern exactly what we want a Should
Should They can be uniquely addressed by a name/segments, but they cannot be run from the CLI (I think?), and do not return any JSON-serializable output (since their whole thing is about keeping stuff in memory for performance and not serializing it) I think regardless of what definition we choose I can probably make the tests pass, so this is more from a philosophical perspective rather than from any practical constraints. What do we think makes the most sense, to us and to other people? |
I guess if we define What about |
Another issue with the naming is that it is confusing to have " We could rename the abstract class from |
@lihaoyi as you said, Workers are some utility to work with state or processes and mostly an implementation detail. They aren't cacheable and can't be invoked from CLI, so they should also not inherit About the names. I think Commands are mostly for use from the CLI, but can also be useful as an API, but only if their parameters accept |
Hmm ok. If we do wish to make |
I managed to merge a bunch more stuff together, so now the type hierarchy is somewhat flatter and simpler: Before:
After:
|
with Target[Int] { | ||
val ctx = ctx0.withSegments(ctx0.segments ++ Seq(ctx0.segment)) | ||
val readWrite = upickle.default.readwriter[Int] | ||
class TestTarget(taskInputs: Seq[Task[Int]], val pure: Boolean)(implicit ctx0: mill.define.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.
This TestTarget
was always a bit weird; unlike normal TargetImpl
s, it didn't wrap a Task[T]
, and instead implemented the Task[T]
behavior itself, to keep the task graph simple for testing purposes.
Now that we've simplified the Task
hierarchy, we need to be a bit more explicit about implementing the task behavior ourselves, hence all these getter/setter forwarders
@@ -40,22 +40,74 @@ abstract class Task[+T] extends Task.Ops[T] with Applyable[Task, T] { | |||
def self: Task[T] = this | |||
} | |||
|
|||
object Task { |
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.
This is just moved up from the bottom of the file, so it is next to the companion trait
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.
Looks good to me. Great change!
This PR makes all of the
T.{apply, input, source, sources, persistent}
functions return the sameTarget
type, so that you can easily override one with another.T.{command, worker}
are still distinct types.Motivation
This allows us to override one with another, e.g. override a
T.sources
with aT.apply
if we want to replace source files with computed sources.Currently, the workaround is to make
T.source
do the computation, as follows:But the status quo is wasteful: it would begin hashing
foo.txt
at the start of every evaluation, it will watchfoo.txt
for changes, etc.. Even though we know it could never change since it's a generated file. This is because we are not allowed to change the method type ofSource
toT[PathRef]
during the override, and thus we have to preserve all theSource
characteristics even though we know they no longer applyWith this PR, we can simply replace the
T.sources
with aT{...}
, and Mill makes use of that information to avoid hashing/watching thePathRef
unnecessarilyImplementation
NamedTask
andNamedTaskImpl
were mergedTarget
's logic was mostly hoisted intoNamedTask
, leavingTarget
an empty marker traitTargetImpl
is unchangedInput
,Source
andSources
have been renamedInputImpl
,SourceImpl
, andSourcesImplt
All the functions that used to return
Source
/Sources
/Persistent
/etc. now return the same typeTarget
, meaning that we can easily override one with the other.I added stubs in
mill/define/package.scala
to make existing type annotations: Sources
,: Input
, etc. continue to work as type aliases toTarget[T]
, and our large codebase and test suite required relatively few changesCommand
/T.command
andWorker
/T.worker
continue to return their specific typeCommand[T]
/Worker[T]
, since they are not sub-types ofTarget[T]
.The
Task
type hierarchy is considerably flatter and simpler:Before:
Task
Task.Sequence
,Task.TraverseCtx
,Task.Mapped
,Task.Zipped
,T.task
, etc.NamedTask
NamedTaskImpl
Command
Worker
Input
Source
Sources
Target
TargetImpl
(also inherits fromNamedTaskImpl
)Persistent
After:
Task
Task.Sequence
,Task.TraverseCtx
,Task.Mapped
,Task.Zipped
,T.task
, etc.NamedTask
Command
Worker
Target
TargetImpl
PersistentImpl
InputImpl
SourceImpl
SourcesImpl
Testing
Added some tests to
TaskTests.scala
to demonstrate and validate the behavior when people override with different types. This was previously a compile errorI had to update the error message for the wrong number of param lists, from
T{...} definitions must have 0 parameter lists
toTarget definitions must have 0 parameter lists
, since we no longer know which target sub-type a method returns based on its signatureNotes