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.

Sign up for GitHub

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

Jump to bottom

CLI: Allow Input from stdin and Output to stdout #701

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CLI: Allow Input from stdin and Output to stdout #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/cli/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,9 @@ pub struct BuildCommandOpts {
/// Path of the JavaScript input file.
pub input: PathBuf,

#[arg(short, default_value = "index.wasm")]
#[arg(short)]
/// Desired path of the WebAssembly output file.
pub output: PathBuf,
pub output: Option<PathBuf>,

#[arg(short = 'C', long = "codegen")]
/// Code generation options.
Expand Down
2 changes: 1 addition & 1 deletion crates/cli/src/js.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub struct JS {
}

impl JS {
fn from_string(source_code: String) -> JS {
pub fn from_string(source_code: String) -> JS {
JS {
source_code: Rc::new(source_code),
}
Expand Down
18 changes: 14 additions & 4 deletions crates/cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use javy_config::Config;
use js::JS;
use std::fs;
use std::fs::File;
use std::io::Write;
use std::io::{Read, Write};

fn main() -> Result<()> {
let args = Cli::parse();
Expand Down Expand Up @@ -58,7 +58,14 @@ fn main() -> Result<()> {
Ok(())
}
Command::Build(opts) => {
let js = JS::from_file(&opts.input)?;
let js = match opts.input.to_str() {
Some("-") => {
Copy link
Member

@saulecabrera saulecabrera Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the technical side of this change, any particular reason why you chose hyphens over:

  • Declaring the input and output options as optional via Option<T>
  • Default to stdin / stdout accordingly if either or both are not specified

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mut content = String::new();
std::io::stdin().read_to_string(&mut content)?;
JS::from_string(content)
}
_ => JS::from_file(&opts.input)?,
};
let codegen: CodegenOptionGroup = opts.codegen.clone().try_into()?;
let mut builder = CodeGenBuilder::new();
builder
Expand All @@ -74,8 +81,11 @@ fn main() -> Result<()> {
};

let wasm = gen.generate(&js)?;

fs::write(&opts.output, wasm)?;
if let Some(path) = opts.output.as_ref() {
fs::write(path, wasm)?;
} else {
std::io::stdout().write_all(&wasm)?;
}
Ok(())
}
}
Expand Down
Loading