refactor: make --full/-f a command-level flag instead of global (#877)
* refactor: make --full/-f a command-level flag instead of global Move --full/-f from global flags (parsed in flags.rs) to command-level parsing in commands.rs, scoped to the three commands that actually use it: `screenshot`, `diff screenshot`, and `diff url`. This frees up `-f` for other commands (e.g. `--follow` on `console`/`errors`, see #867) and better reflects that full-page capture is not a global concern. Changes: - Remove `full` from Flags struct, Config struct, and global flag parsing - Remove `--full`/`-f` from clean_args global boolean flags list - Parse `--full`/`-f` inline in `screenshot` command handler - Accept `-f` shorthand in `diff screenshot` and `diff url` (previously only `--full` was accepted at command level) - Remove fallback from global `flags.full` in diff subcommands - Update tests to pass --full as a command argument rather than a global flag Fixes #876 * fix: remove stale AGENT_BROWSER_FULL env var from help and add -f shorthand tests - Remove AGENT_BROWSER_FULL from help text in output.rs since the env var is no longer read after moving --full to command-level parsing - Add test_screenshot_full_page_shorthand to verify screenshot -f works - Add test_diff_screenshot_command_full_flag_shorthand to verify diff screenshot -f works --------- Co-authored-by: ctate <366502+ctate@users.noreply.github.com>
This commit is contained in:
+54
-29
@@ -449,10 +449,22 @@ pub fn parse_command(args: &[String], flags: &Flags) -> Result<Value, ParseError
|
||||
|
||||
// === Screenshot/PDF ===
|
||||
"screenshot" => {
|
||||
// screenshot [selector] [path]
|
||||
// screenshot [selector] [path] [--full/-f]
|
||||
// selector: @ref or CSS selector
|
||||
// path: file path (contains / or . or ends with known extension)
|
||||
let (selector, path) = match (rest.first(), rest.get(1)) {
|
||||
let mut full_page = false;
|
||||
let positional: Vec<&str> = rest
|
||||
.iter()
|
||||
.filter(|arg| match **arg {
|
||||
"--full" | "-f" => {
|
||||
full_page = true;
|
||||
false
|
||||
}
|
||||
_ => true,
|
||||
})
|
||||
.copied()
|
||||
.collect();
|
||||
let (selector, path) = match (positional.first(), positional.get(1)) {
|
||||
(Some(first), Some(second)) => {
|
||||
// Two args: first is selector, second is path
|
||||
(Some(*first), Some(*second))
|
||||
@@ -480,7 +492,7 @@ pub fn parse_command(args: &[String], flags: &Flags) -> Result<Value, ParseError
|
||||
let mut cmd = json!({
|
||||
"id": id, "action": "screenshot",
|
||||
"path": path, "selector": selector,
|
||||
"fullPage": flags.full, "annotate": flags.annotate
|
||||
"fullPage": full_page, "annotate": flags.annotate
|
||||
});
|
||||
if let Some(ref fmt) = flags.screenshot_format {
|
||||
cmd["format"] = json!(fmt);
|
||||
@@ -1315,7 +1327,7 @@ pub fn parse_command(args: &[String], flags: &Flags) -> Result<Value, ParseError
|
||||
}
|
||||
}
|
||||
|
||||
"diff" => parse_diff(&rest, &id, flags),
|
||||
"diff" => parse_diff(&rest, &id),
|
||||
|
||||
// === Batch ===
|
||||
"batch" => {
|
||||
@@ -1329,7 +1341,7 @@ pub fn parse_command(args: &[String], flags: &Flags) -> Result<Value, ParseError
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_diff(rest: &[&str], id: &str, flags: &Flags) -> Result<Value, ParseError> {
|
||||
fn parse_diff(rest: &[&str], id: &str) -> Result<Value, ParseError> {
|
||||
const VALID: &[&str] = &["snapshot", "screenshot", "url"];
|
||||
|
||||
match rest.first().copied() {
|
||||
@@ -1474,27 +1486,24 @@ fn parse_diff(rest: &[&str], id: &str, flags: &Flags) -> Result<Value, ParseErro
|
||||
});
|
||||
}
|
||||
}
|
||||
"--full" => {
|
||||
"--full" | "-f" => {
|
||||
obj.insert("fullPage".to_string(), json!(true));
|
||||
}
|
||||
other if other.starts_with('-') => {
|
||||
return Err(ParseError::InvalidValue {
|
||||
message: format!("Unknown flag: {}", other),
|
||||
usage: "diff screenshot --baseline <file> [--output <file>] [--threshold <0-1>] [--selector <sel>] [--full]",
|
||||
usage: "diff screenshot --baseline <file> [--output <file>] [--threshold <0-1>] [--selector <sel>] [--full/-f]",
|
||||
});
|
||||
}
|
||||
other => {
|
||||
return Err(ParseError::InvalidValue {
|
||||
message: format!("Unexpected argument: {}", other),
|
||||
usage: "diff screenshot --baseline <file> [--output <file>] [--threshold <0-1>] [--selector <sel>] [--full]",
|
||||
usage: "diff screenshot --baseline <file> [--output <file>] [--threshold <0-1>] [--selector <sel>] [--full/-f]",
|
||||
});
|
||||
}
|
||||
}
|
||||
i += 1;
|
||||
}
|
||||
if flags.full {
|
||||
obj.insert("fullPage".to_string(), json!(true));
|
||||
}
|
||||
if !obj.contains_key("baseline") {
|
||||
return Err(ParseError::MissingArguments {
|
||||
context: "diff screenshot".to_string(),
|
||||
@@ -1525,7 +1534,7 @@ fn parse_diff(rest: &[&str], id: &str, flags: &Flags) -> Result<Value, ParseErro
|
||||
"--screenshot" => {
|
||||
obj.insert("screenshot".to_string(), json!(true));
|
||||
}
|
||||
"--full" => {
|
||||
"--full" | "-f" => {
|
||||
obj.insert("fullPage".to_string(), json!(true));
|
||||
}
|
||||
"--wait-until" => {
|
||||
@@ -1580,21 +1589,18 @@ fn parse_diff(rest: &[&str], id: &str, flags: &Flags) -> Result<Value, ParseErro
|
||||
other if other.starts_with('-') => {
|
||||
return Err(ParseError::InvalidValue {
|
||||
message: format!("Unknown flag: {}", other),
|
||||
usage: "diff url <url1> <url2> [--screenshot] [--full] [--wait-until <strategy>] [--selector <sel>] [--compact] [--depth <n>]",
|
||||
usage: "diff url <url1> <url2> [--screenshot] [--full/-f] [--wait-until <strategy>] [--selector <sel>] [--compact] [--depth <n>]",
|
||||
});
|
||||
}
|
||||
other => {
|
||||
return Err(ParseError::InvalidValue {
|
||||
message: format!("Unexpected argument: {}", other),
|
||||
usage: "diff url <url1> <url2> [--screenshot] [--full] [--wait-until <strategy>] [--selector <sel>] [--compact] [--depth <n>]",
|
||||
usage: "diff url <url1> <url2> [--screenshot] [--full/-f] [--wait-until <strategy>] [--selector <sel>] [--compact] [--depth <n>]",
|
||||
});
|
||||
}
|
||||
}
|
||||
i += 1;
|
||||
}
|
||||
if flags.full {
|
||||
obj.insert("fullPage".to_string(), json!(true));
|
||||
}
|
||||
Ok(cmd)
|
||||
}
|
||||
Some(sub) => Err(ParseError::UnknownSubcommand {
|
||||
@@ -2165,7 +2171,6 @@ mod tests {
|
||||
Flags {
|
||||
session: "test".to_string(),
|
||||
json: false,
|
||||
full: false,
|
||||
headed: false,
|
||||
debug: false,
|
||||
headers: None,
|
||||
@@ -2728,9 +2733,14 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_screenshot_full_page() {
|
||||
let mut flags = default_flags();
|
||||
flags.full = true;
|
||||
let cmd = parse_command(&args("screenshot"), &flags).unwrap();
|
||||
let cmd = parse_command(&args("screenshot --full"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "screenshot");
|
||||
assert_eq!(cmd["fullPage"], true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_screenshot_full_page_shorthand() {
|
||||
let cmd = parse_command(&args("screenshot -f"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "screenshot");
|
||||
assert_eq!(cmd["fullPage"], true);
|
||||
}
|
||||
@@ -3598,10 +3608,23 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_diff_screenshot_global_full_flag() {
|
||||
let mut flags = default_flags();
|
||||
flags.full = true;
|
||||
let cmd = parse_command(&args("diff screenshot --baseline b.png"), &flags).unwrap();
|
||||
fn test_diff_screenshot_command_full_flag() {
|
||||
let cmd = parse_command(
|
||||
&args("diff screenshot --baseline b.png --full"),
|
||||
&default_flags(),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(cmd["action"], "diff_screenshot");
|
||||
assert_eq!(cmd["fullPage"], true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_diff_screenshot_command_full_flag_shorthand() {
|
||||
let cmd = parse_command(
|
||||
&args("diff screenshot --baseline b.png -f"),
|
||||
&default_flags(),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(cmd["action"], "diff_screenshot");
|
||||
assert_eq!(cmd["fullPage"], true);
|
||||
}
|
||||
@@ -3642,10 +3665,12 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_diff_url_global_full_flag() {
|
||||
let mut flags = default_flags();
|
||||
flags.full = true;
|
||||
let cmd = parse_command(&args("diff url https://a.com https://b.com"), &flags).unwrap();
|
||||
fn test_diff_url_command_full_flag() {
|
||||
let cmd = parse_command(
|
||||
&args("diff url https://a.com https://b.com --full"),
|
||||
&default_flags(),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(cmd["fullPage"], true);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user