CLI: Allow Input from stdin and Output to stdout#701
CLI: Allow Input from stdin and Output to stdout#701konradkoschel wants to merge 1 commit intobytecodealliance:mainfrom
Conversation
There was a problem hiding this comment.
Thanks for the PR -- generally these changes seem reasonable to me. One thing to note is that we're currently thinking of revamping Javy's CLI interface (see #702).
Given that our intention is to move in the direction described in the issue linked above, it probably makes sense to include this change as part of the new build command. That said, you're not expected to take on the CLI changes, for this change to land. However, I'd like to ask if you're ok waiting until we at least introduce the command that will replace the current compile command in order to merge this?
| Command::Compile(opts) => { | ||
| let js = JS::from_file(&opts.input)?; | ||
| let js = match opts.input.to_str() { | ||
| Some("-") => { |
There was a problem hiding this comment.
On the technical side of this change, any particular reason why you chose hyphens over:
- Declaring the
inputandoutputoptions as optional viaOption<T> - Default to
stdin/stdoutaccordingly if either or both are not specified
There was a problem hiding this comment.
For stdin input, I would argue that it is common practice to use a hyphen as the argument to indicate this explicitely. For the output, I didn't want to break the current default behavior (i.e. using index.wasm).
I see the point though, that especially the combination of both (-o - - / - -o -) may look a bit weird. In the end, I wanted to primarily make a proposal for the functionality – the implementation details can be discussed.
As this will anyway be implemented as part of the new build command, I would default the output to stdout as suggested by you.
Hi, thanks for the fast response! I agree on the point that it makes sense to incorporate the functionality into the upcoming |
|
Hi @konradkoschel, wanted to let you know that most of the backing infrastructure for the CLI redesign has landed on main, and I think it's now a good time to rebase, update and move forward with this change. Let me know if you have any questions. |
981686e to
d894a54
Compare
Hi Saul, I updated the branch. Could you check if the code changes are fine? As discussed previously, I made the binary output the default instead of |
saulecabrera
left a comment
There was a problem hiding this comment.
Code changes look good to me. Would you mind adding a test or two here https://github.com/bytecodealliance/javy/blob/main/crates/cli/tests/integration_test.rs to test the new behavior?
Description of the change
In this PR, the CLI crate is enhanced so that a hyphen (
-) can be passed for the input argument or the output option instead of a file name indicating that stdio streams should be used.Possible usage
Why am I making this change?
I built a wrapper around Javy. The wrapper is not inherently working with files, but to make it work with Javy, I need to write the JavaScript to the filesystem and read the WASM from the filesystem.
Generally, the changes make the CLI of Javy more flexible.
Checklist
javy-cliandjavy-coredo not require updating CHANGELOG files.