Make input tasks only require a JSON writer, not reader #1601
Conversation
lihaoyi
changed the title
| def format = task match { | ||
| case t: Target[T] => Some(t.readWrite.asInstanceOf[upickle.default.ReadWriter[T]]) | ||
| case _ => None | ||
| } | ||
| def writer = task match { | ||
| case t: mill.define.Command[T] => Some(t.writer.asInstanceOf[upickle.default.Writer[T]]) | ||
| case t: Target[T] => Some(t.readWrite.asInstanceOf[upickle.default.ReadWriter[T]]) | ||
| case _ => None | ||
| } |
There was a problem hiding this comment.
These pattern matches are kind of weird, and go against the un-sealed open nature of our Task class hierarchy. Thus I moved the logic into abstract methods on NamedTask.
But TBH I'm not sure if that's the right thing to do. We really should decide if Task is meant to be a closed sealed-trait/case-class hierarchy operated on with pattern matches or an open trait/class hierarchy operated on by inheritance and virtual method dispatch...
There was a problem hiding this comment.
Until now I always though of our tasks as a sealed hierarchy. We handle most of their specialities when evaluating them and I find it hard to come up with scenarios which could be realized by a new type of task, which can be solely realized through the tasks implementation. So we should make them sealed.
lihaoyi
changed the title
|
@lefou I reverted the pattern-match/virtual-methods change for now. It's orthogonal to the main goal of this PR, and I'm not yet confident enough to decide either way so easiest to just leave as is for now |
There was a problem hiding this comment.
I really like this change, as it removes an unnecessary restriction. And this issue already popped up in the chats.
I still have some questions to the implementation touched in this PR but not directly to this change. So feel free to address it or not. The Labelled case class has a writer method which is used to write, but it has also a format method. If I'm right, this one is only used to read stuff? I don't get why it is called format? Maybe you can take the opportunity to a) add proper return types (to make clear if we are also interested in the inferred writer-aspect or not) and b) probably rename it to read (or enlighten me why not).
|
The name We should clean it up, but I'd like to get this fix in first and we can clean up separately. As discussed, we should probably do something about the |
…1601) Resolves com-lihaoyi#598. Input tasks results are re-evaluated every time and thus never read from the disk, and so a `upickle.default.Reader` is not required. Nevertheless, we still require that a `upickle.default.Writer` in order to generate the `*.json` files used for `./mill show` and other debugging purposes. Targeting com-lihaoyi#1600 to avoid merge conflicts Let's see what CI says... TBH not sure if this is worth doing, or whether we should just specify that `Input` tasks require a `ReadWriter` just to keep things consistent. We don't currently have any use case for de-serializing the input task JSON (e.g. even `mill show` takes the JSON from disk and spits it out verbatim without de-serializing) but that doesn't mean we won't find such use cases in future. But if such use cases do re-appear, we can always add that functionality back. `Command`s are write-only, so making `Input`s write-only would also not be unprecedented
Resolves #598. Input tasks results are re-evaluated every time and thus never read from the disk, and so a
upickle.default.Readeris not required. Nevertheless, we still require that aupickle.default.Writerin order to generate the*.jsonfiles used for./mill showand other debugging purposes.Targeting #1600 to avoid merge conflicts
Let's see what CI says...
TBH not sure if this is worth doing, or whether we should just specify that
Inputtasks require aReadWriterjust to keep things consistent. We don't currently have any use case for de-serializing the input task JSON (e.g. evenmill showtakes the JSON from disk and spits it out verbatim without de-serializing) but that doesn't mean we won't find such use cases in future. But if such use cases do re-appear, we can always add that functionality back.Commands are write-only, so makingInputs write-only would also not be unprecedented