Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

#### :nail_care: Polish

- Rewatch cli: do not show build command options in the root help. https://github.com/rescript-lang/rescript/pull/7715

#### :house: Internal

# 12.0.0-beta.13
Expand Down
11 changes: 5 additions & 6 deletions rewatch/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ pub enum FileExtension {
/// ReScript - Fast, Simple, Fully Typed JavaScript from the Future
#[derive(Parser, Debug)]
#[command(version)]
#[command(args_conflicts_with_subcommands = true)]
#[command(
after_help = "Note: If no command is provided, the build command is run by default. See `rescript help build` for more information."
)]
pub struct Cli {
/// Verbosity:
/// -v -> Debug
Expand All @@ -35,10 +37,7 @@ pub struct Cli {

/// The command to run. If not provided it will default to build.
#[command(subcommand)]
pub command: Option<Command>,

#[command(flatten)]
pub build_args: BuildArgs,
pub command: Command,
}

#[derive(Args, Debug, Clone)]
Expand Down Expand Up @@ -181,7 +180,7 @@ impl From<BuildArgs> for WatchArgs {

#[derive(Subcommand, Clone, Debug)]
pub enum Command {
/// Build the project
/// Build the project (default command)
Build(BuildArgs),
/// Build, then start a watcher
Watch(WatchArgs),
Expand Down
161 changes: 156 additions & 5 deletions rewatch/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
use anyhow::Result;
use clap::Parser;
use clap::{CommandFactory, Parser, error::ErrorKind};
use log::LevelFilter;
use std::{io::Write, path::Path};
use std::{env, ffi::OsString, io::Write, path::Path};

use rescript::{build, cli, cmd, format, lock, watcher};

fn main() -> Result<()> {
let args = cli::Cli::parse();
// Use `args_os` so non-UTF bytes still reach clap for proper error reporting on platforms that
// allow arbitrary argv content.
let raw_args: Vec<OsString> = env::args_os().collect();
let cli = parse_cli(raw_args).unwrap_or_else(|err| err.exit());

let log_level_filter = args.verbose.log_level_filter();
let log_level_filter = cli.verbose.log_level_filter();

env_logger::Builder::new()
.format(|buf, record| writeln!(buf, "{}:\n{}", record.level(), record.args()))
.filter_level(log_level_filter)
.target(env_logger::fmt::Target::Stdout)
.init();

let mut command = args.command.unwrap_or(cli::Command::Build(args.build_args));
let mut command = cli.command;

if let cli::Command::Build(build_args) = &command {
if build_args.watch {
Expand Down Expand Up @@ -111,3 +114,151 @@ fn get_lock(folder: &str) -> lock::Lock {
acquired_lock => acquired_lock,
}
}

fn parse_cli(raw_args: Vec<OsString>) -> Result<cli::Cli, clap::Error> {
Copy link
Member

Choose a reason for hiding this comment

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

Makes me wonder if we can check if the first argument is any of the verbs, if not prepend the vector with build.
That way we avoid the double parse in the error case.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I had in the beginning, and it had other issues.
See the previous codex reviews, it took quite a few attempts until everything worked correctly.

match cli::Cli::try_parse_from(&raw_args) {
Ok(cli) => Ok(cli),
Err(err) => {
if should_default_to_build(&err, &raw_args) {
let mut fallback_args = raw_args.clone();
let insert_at = index_after_global_flags(&fallback_args);
fallback_args.insert(insert_at, OsString::from("build"));

Choose a reason for hiding this comment

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

[P1] Default build fallback rejects global flags after folder paths

When the user omits an explicit subcommand, parse_cli inserts the implicit build token after only the leading global flags. Global flags that appear after the folder argument (e.g. rescript my-project -v or rescript ./ --quiet) are left trailing the newly inserted build token, so clap parses them as arguments to the build subcommand and reports UnknownArgument. The pre‑existing CLI accepted these invocations because there was no subcommand and global flags could be positioned after the folder. This regression will break scripts that pass -v/--quiet after the path. Consider collecting global flags regardless of position or reordering them ahead of the injected build command before reparsing.

Useful? React with 👍 / 👎.


match cli::Cli::try_parse_from(&fallback_args) {
Ok(cli) => Ok(cli),
Err(fallback_err) => Err(fallback_err),
}
} else {
Err(err)
}
}
}
}

fn should_default_to_build(err: &clap::Error, args: &[OsString]) -> bool {
match err.kind() {
ErrorKind::MissingSubcommand
| ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand
| ErrorKind::UnknownArgument
| ErrorKind::InvalidSubcommand => {
let first_non_global = first_non_global_arg(args);
match first_non_global {
Some(arg) => !is_known_subcommand(arg),
None => true,
}
}
_ => false,
}
}

fn index_after_global_flags(args: &[OsString]) -> usize {
let mut idx = 1;
while let Some(arg) = args.get(idx) {
if is_global_flag(arg) {
idx += 1;
} else {
break;
}
}
idx.min(args.len())
}

fn is_global_flag(arg: &OsString) -> bool {
matches!(
arg.to_str(),
Some(
"-v" | "-vv"
| "-vvv"
| "-vvvv"
| "-q"
| "-qq"
| "-qqq"
| "-qqqq"
| "--verbose"
| "--quiet"
| "-h"
| "--help"
| "-V"
| "--version"
)
)
}

fn first_non_global_arg(args: &[OsString]) -> Option<&OsString> {
args.iter().skip(1).find(|arg| !is_global_flag(arg))
}

fn is_known_subcommand(arg: &OsString) -> bool {
let Some(arg_str) = arg.to_str() else {
return false;
};

cli::Cli::command().get_subcommands().any(|subcommand| {
subcommand.get_name() == arg_str || subcommand.get_all_aliases().any(|alias| alias == arg_str)
})
}

#[cfg(test)]
mod tests {
use super::*;
use std::ffi::OsString;

fn parse(args: &[&str]) -> Result<cli::Cli, clap::Error> {
parse_cli(args.iter().map(OsString::from).collect())
}

#[test]
fn defaults_to_build_without_args() {
let cli = parse(&["rescript"]).expect("expected default build command");

match cli.command {
cli::Command::Build(build_args) => assert_eq!(build_args.folder.folder, "."),
other => panic!("expected build command, got {other:?}"),
}
}

#[test]
fn defaults_to_build_with_folder_shortcut() {
let cli = parse(&["rescript", "someFolder"]).expect("expected build command");

match cli.command {
cli::Command::Build(build_args) => assert_eq!(build_args.folder.folder, "someFolder"),
other => panic!("expected build command, got {other:?}"),
}
}

#[test]
fn respects_global_flag_before_subcommand() {
let cli = parse(&["rescript", "-v", "watch"]).expect("expected watch command");

assert!(matches!(cli.command, cli::Command::Watch(_)));
}

#[test]
fn invalid_option_for_subcommand_does_not_fallback() {
let err = parse(&["rescript", "watch", "--no-timing"]).expect_err("expected watch parse failure");
assert_eq!(err.kind(), ErrorKind::UnknownArgument);
}

#[test]
fn help_flag_does_not_default_to_build() {
let err = parse(&["rescript", "--help"]).expect_err("expected clap help error");
assert_eq!(err.kind(), ErrorKind::DisplayHelp);
}

#[test]
fn version_flag_does_not_default_to_build() {
let err = parse(&["rescript", "--version"]).expect_err("expected clap version error");
assert_eq!(err.kind(), ErrorKind::DisplayVersion);
}

#[cfg(unix)]
#[test]
fn non_utf_argument_returns_error() {
use std::os::unix::ffi::OsStringExt;

let args = vec![OsString::from("rescript"), OsString::from_vec(vec![0xff])];
let err = parse_cli(args).expect_err("expected clap to report invalid utf8");
assert_eq!(err.kind(), ErrorKind::InvalidUtf8);
}
}
Loading