SPICE-0025: pkl run CLI framework

[HT154]

No description provided.

[bioball]

Is this referring to trace() (writes to standard error) and read("http://...") (makes HTTP requests), or something else?

[bioball]

Why not Listing and Mapping?

[HT154]

What would the motivation be for providing these as Object types? In either case their CLI-provided values will be eager and constant. The API (and capabilities like for generators and spreading) of List/Map are strict supersets of Listing/Mapping.

This could be supported, but it would slightly complicate the implementation.

[bioball]

I like it because you can treat the options object as normal data, and amend it, etc.

[bioball]

Mmm norway problem? Can we just accept true or false instead?

Alternatively, might be good to just have no value, so you can do something like: pkl run myMod.pkl --silent instead of pkl run myMod.pkl --silent=true.

I think it feels okay to accept both true/false values, and/or just treat this as a boolean flag. So both of these would work:

--silent true
--silent

[HT154]

Mmm norway problem? Can we just accept true or false instead?

This is actually vanilla clikt behavior here: it accepts all these values for boolean options. This is easy to change if we want to be stricter here.

Alternatively, might be good to just have no value, so you can do something like: pkl run myMod.pkl --silent instead of pkl run myMod.pkl --silent=true.

I considered this, but it is somewhat inconsistent with other types. I'm open to changing this though.

I think it feels okay to accept both true/false values, and/or just treat this as a boolean flag.

This is not possible to do unambiguously with clikt: ajalt/clikt#280

[bioball]

Boolean flags are so common that I feel like we should support this somehow.

We can maybe introduce a new specialized flag for it? Where the property's type must be Boolean?

@BooleanFlag
foo: Boolean

[bioball]

I think there is an ambiguity here.

I don't think this is stated anywhere in this SPICE, but I'm interpreting that List/Set/Map are meant to be treated as repeatable arguments and flags.

Given this, doesn't seem possible to express "a single flag that I want parsed as a List". E.g.

@Flag { parse = (it) -> it.split(",") }
singleFlag: List<String>

Either you must accept repeatable flags/args (and make this a 2d structure), or you must accept a raw string and then parse downstream.

I think we can resolve this ambiguity by requiring that this is explicitly declared, for example, by using another annotation:

@Repeatable
@Flag
repeatableFlag: List<String>

@Flag { parse = (it) -> it.split(",") }
singleFlag: List<String>

[bioball]

Should we allow short names to be combine-able? Like how tar -xvf is a shorthand for tar -x -v -f?

How would you express something like curl -v, curl -vv, curl -vvv (more v's means more verbose)? Do we care to support this?

[HT154]

Should we allow short names to be combine-able? Like how tar -xvf is a shorthand for tar -x -v -f?

This is already supported because clikt supports is. Because this doesn't support boolean flags without a value (what clikt calls "flags" vs. just "options") this isn't obvious.

How would you express something like curl -v, curl -vv, curl -vvv (more v's means more verbose)? Do we care to support this?

This is relatively niche. Clikt calls these "counted flags". I'm not sure there's a really strong motivation to support this, but it's certainly possible.

[bioball]

This is already supported because clikt supports is. Because this doesn't support boolean flags without a value (what clikt calls "flags" vs. just "options") this isn't obvious.

Cool! And, can we document this? Since this is our feature, and Clikt is an implementation detail.

This is relatively niche. Clikt calls these "counted flags". I'm not sure there's a really strong motivation to support this, but it's certainly possible.

I think the "counted flags" stuff can be supported if we have boolean flags and repeatable flags, since -vvv can just desugar to -v -v -v. But, I agree, it's not worth its own feature.

[bioball]

This means that you cannot create an executable that also supports Pkl options.

For example, this wouldn't work:

some_executable.pkl --root-dir  ./

This seems like a somewhat big drawback.

This also means that these two flags mean different things:

pkl run --root-dir ./ foo.pkl
pkl run foo.pkl --root-dir ./

Which feels somewhat footgun-ish to me? It feels easy to mess up, and has security implications.

[bioball]

I don't think it's worth changing the type just for better interop with glob imports.

For glob imports, you can just do:

subcommands {
  for (command in import*(...)) {
    command
  }
}

[HT154]

Hmm the overhead for supporting this is extremely low (there is zero handling in the implementation for this since it calls iterateAlreadyForcedMemberValues on the object and ignores the key.

I don't think there's harm in retaining support for this, especially since a single glob import is likely to be a common pattern for defining subcommands.

[bioball]

In general, we try to use the appropriate data structure without considering how they interact with language features. Language features evolve, and we can improve the interaction story here in the future (like adding .values on Mapping perhaps). If we accept Mapping here, we'd have a data structure where the key doesn't represent anything here, e.g.

subcommands {
  ["what does this key mean?"] { /* etc */ }
}

The same can be said of the tests section of a PklProject; we could have made it accept a Mapping too:

tests: Mapping<String, test>

// then later..

tests = import*("tests/**.pkl")

But, the value wouldn't be used for anything here, and, so, wouldn't be appropriate.

[bioball]

How do commands affect exit code? Should they be allowed to?

[HT154]

Commands can return a failure using throw (exit code 1), but otherwise may not control the exit code.

[bioball]

I wonder if we should be using Module#output? Maybe we should have explicitly set properties instead?

module pkl.Command

stdout: Bytes?

files: Mapping<String, FileOutput>?

exitCode: Int = 0

This is nice because we can add more here to control the behavior of pkl run in the future.

[HT154]

Hmm I'm skeptical of doing this because it increases the distance between command modules and "regular" modules and the similarity is powerful.

An intermediate approach here might be to open ModuleOutput and subclass it (or subclass FileOutput) as CommandOutput and override the type of output.