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

Input tasks require JSON readable and writable output #598

Closed
nrktkt opened this issue Apr 18, 2019 · 2 comments · Fixed by #1601
Closed

Input tasks require JSON readable and writable output #598

nrktkt opened this issue Apr 18, 2019 · 2 comments · Fixed by #1601
Milestone

Comments

@nrktkt
Copy link
Contributor

nrktkt commented Apr 18, 2019

I'm not sure what the intended behavior is, but the there's a little inconsistency between the documentation and the code for input tasks.
The task cheat sheet confirms that inputs do not need to be JSON readable or JSON writable, however T.input requires an implicit upickle ReadWriter for the result type.

steps to reproduce:

import mill._, scalalib._
import java.time.Instant

object baz extends JavaModule {
  def test = T.input(Instant.now)
}

Confirm the build does not compile due to

could not find implicit value for parameter rw: upickle.default.ReadWriter[java.time.Instant]

@lihaoyi
Copy link
Member

lihaoyi commented Apr 19, 2019

Not sure how easy it would be to remove the ReadWriter dependency in Input, but one workaround could be to define a ReadWriter for java.time.Instant. Since it's in the standard library, perhaps even push it upstream to the uPickle repo

@nrktkt
Copy link
Contributor Author

nrktkt commented Apr 19, 2019

The workaround I'm currently using is

def w[T] = writer[Unit].comap[T](_ => ())
def r[T] = reader[Unit].map[T](_ => ???)
def rw[T]: ReadWriter[T] = ReadWriter.join(r, w)
def task = T.input(...)(rw, implicitly[Ctx])

For two reasons

  1. This works for any type, and as far as I can tell the actual JSON value is unused.
  2. Some of the values I want to use are sensitive (db credentials for the flyway plugin) and I don't want them written to disk unnecessarily.

This second point could probably be expanded into a separate feature request except that the documented behavior of an input task already does pretty much what I want.

lihaoyi added a commit that referenced this issue Dec 8, 2021
Resolves #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 #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
@lefou lefou added this to the after 0.10.0-M4 milestone Dec 8, 2021
pbuszka pushed a commit to pbuszka/mill that referenced this issue Dec 26, 2021
…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
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 a pull request may close this issue.

3 participants