Skip to content

fmt: fix error priority, make goal-errors more helpful #6362

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

Merged
merged 1 commit into from
May 7, 2024
Merged

fmt: fix error priority, make goal-errors more helpful #6362

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
61 changes: 41 additions & 20 deletions src/uu/fmt/src/fmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,24 @@ impl FmtOptions {
.get_one::<String>(options::SKIP_PREFIX)
.map(String::from);

let width_opt = extract_width(matches);
let goal_opt = matches.get_one::<usize>(options::GOAL);
let width_opt = extract_width(matches)?;
let goal_opt_str = matches.get_one::<String>(options::GOAL);
let goal_opt = if let Some(goal_str) = goal_opt_str {
match goal_str.parse::<usize>() {
Ok(goal) => Some(goal),
Err(_) => {
return Err(USimpleError::new(
1,
format!("invalid goal: {}", goal_str.quote()),
));
}
}
} else {
None
};

let (width, goal) = match (width_opt, goal_opt) {
(Some(w), Some(&g)) => {
(Some(w), Some(g)) => {
if g > w {
return Err(USimpleError::new(1, "GOAL cannot be greater than WIDTH."));
}
Expand All @@ -101,7 +115,7 @@ impl FmtOptions {
let g = (w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100).max(if w == 0 { 0 } else { 1 });
(w, g)
}
(None, Some(&g)) => {
(None, Some(g)) => {
if g > DEFAULT_WIDTH {
return Err(USimpleError::new(1, "GOAL cannot be greater than WIDTH."));
}
Expand Down Expand Up @@ -243,22 +257,29 @@ fn extract_files(matches: &ArgMatches) -> UResult<Vec<String>> {
}
}

fn extract_width(matches: &ArgMatches) -> Option<usize> {
let width_opt = matches.get_one::<usize>(options::WIDTH);
if let Some(width) = width_opt {
return Some(*width);
fn extract_width(matches: &ArgMatches) -> UResult<Option<usize>> {
let width_opt = matches.get_one::<String>(options::WIDTH);
if let Some(width_str) = width_opt {
if let Ok(width) = width_str.parse::<usize>() {
return Ok(Some(width));
} else {
return Err(USimpleError::new(
1,
format!("invalid width: {}", width_str.quote()),
));
}
}

if let Some(1) = matches.index_of(options::FILES_OR_WIDTH) {
let width_arg = matches.get_one::<String>(options::FILES_OR_WIDTH).unwrap();
if let Some(num) = width_arg.strip_prefix('-') {
num.parse::<usize>().ok()
Ok(num.parse::<usize>().ok())
} else {
// will be treated as a file name
None
Ok(None)
}
} else {
None
Ok(None)
}
}

Expand Down Expand Up @@ -406,16 +427,16 @@ pub fn uu_app() -> Command {
.short('w')
.long("width")
.help("Fill output lines up to a maximum of WIDTH columns, default 75. This can be specified as a negative number in the first argument.")
.value_name("WIDTH")
.value_parser(clap::value_parser!(usize)),
// We must accept invalid values if they are overridden later. This is not supported by clap, so accept all strings instead.
.value_name("WIDTH"),
)
.arg(
Arg::new(options::GOAL)
.short('g')
.long("goal")
.help("Goal width, default of 93% of WIDTH. Must be less than or equal to WIDTH.")
.value_name("GOAL")
.value_parser(clap::value_parser!(usize)),
// We must accept invalid values if they are overridden later. This is not supported by clap, so accept all strings instead.
.value_name("GOAL"),
)
.arg(
Arg::new(options::QUICK)
Expand Down Expand Up @@ -460,7 +481,7 @@ mod tests {

assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]);

assert_eq!(extract_width(&matches), Some(3));
assert_eq!(extract_width(&matches).ok(), Some(Some(3)));

Ok(())
}
Expand All @@ -471,7 +492,7 @@ mod tests {

assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]);

assert_eq!(extract_width(&matches), Some(3));
assert_eq!(extract_width(&matches).ok(), Some(Some(3)));

Ok(())
}
Expand All @@ -482,7 +503,7 @@ mod tests {

assert_eq!(extract_files(&matches).unwrap(), vec!["-"]);

assert_eq!(extract_width(&matches), None);
assert_eq!(extract_width(&matches).ok(), Some(None));

Ok(())
}
Expand All @@ -493,7 +514,7 @@ mod tests {

assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]);

assert_eq!(extract_width(&matches), None);
assert_eq!(extract_width(&matches).ok(), Some(None));

Ok(())
}
Expand All @@ -504,7 +525,7 @@ mod tests {

assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]);

assert_eq!(extract_width(&matches), Some(3));
assert_eq!(extract_width(&matches).ok(), Some(Some(3)));
Ok(())
}
}
45 changes: 43 additions & 2 deletions tests/by-util/test_fmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,21 @@ fn test_fmt_width() {
.stdout_is("this is a\nfile with\none word\nper line\n");
}

#[test]
fn test_fmt_width_invalid() {
new_ucmd!()
.args(&["one-word-per-line.txt", "-w", "apple"])
.fails()
.code_is(1)
.no_stdout()
.stderr_is("fmt: invalid width: 'apple'\n");
// an invalid width can be successfully overwritten later:
new_ucmd!()
.args(&["one-word-per-line.txt", "-w", "apple", "-w10"])
.succeeds()
.stdout_is("this is a\nfile with\none word\nper line\n");
}

#[test]
fn test_fmt_positional_width() {
new_ucmd!()
Expand Down Expand Up @@ -84,7 +99,7 @@ fn test_fmt_invalid_width() {
.args(&["one-word-per-line.txt", param, "invalid"])
.fails()
.code_is(1)
.stderr_contains("invalid value 'invalid'");
.stderr_contains("invalid width: 'invalid'");
}
}

Expand Down Expand Up @@ -182,10 +197,36 @@ fn test_fmt_invalid_goal() {
.args(&["one-word-per-line.txt", param, "invalid"])
.fails()
.code_is(1)
.stderr_contains("invalid value 'invalid'");
// GNU complains about "invalid width", which is confusing.
// We intentionally deviate from GNU, and show a more helpful message:
.stderr_contains("invalid goal: 'invalid'");
}
}

#[test]
fn test_fmt_invalid_goal_override() {
new_ucmd!()
.args(&["one-word-per-line.txt", "-g", "apple", "-g", "74"])
.succeeds()
.stdout_is("this is a file with one word per line\n");
}

#[test]
fn test_fmt_invalid_goal_width_priority() {
new_ucmd!()
.args(&["one-word-per-line.txt", "-g", "apple", "-w", "banana"])
.fails()
.code_is(1)
.no_stdout()
.stderr_is("fmt: invalid width: 'banana'\n");
new_ucmd!()
.args(&["one-word-per-line.txt", "-w", "banana", "-g", "apple"])
.fails()
.code_is(1)
.no_stdout()
.stderr_is("fmt: invalid width: 'banana'\n");
}

#[test]
fn test_fmt_set_goal_not_contain_width() {
for param in ["-g", "--goal"] {
Expand Down