Skip to content

fmt: implement Default for FmtOptions #4747

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
tertsdiepraam merged 3 commits into from
Aug 11, 2023
Merged

fmt: implement Default for FmtOptions #4747

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42d6604
fmt: implement default for FmtOptions
cakebaker Apr 18, 2023
09db8d8
fmt: adapt defaults to the ones used by GNU fmt
cakebaker Apr 28, 2023
a3b80e1
Merge branch 'main' into fmt_implement_default_for_fmtoptions
tertsdiepraam Aug 11, 2023
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
59 changes: 38 additions & 21 deletions src/uu/fmt/src/fmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,29 @@ pub struct FmtOptions {
goal: usize,
tabwidth: usize,
}

impl Default for FmtOptions {
fn default() -> Self {
Self {
crown: false,
tagged: false,
mail: false,
uniform: false,
quick: false,
split_only: false,
use_prefix: false,
prefix: String::new(),
xprefix: false,
use_anti_prefix: false,
anti_prefix: String::new(),
xanti_prefix: false,
width: 75,
goal: 70,
tabwidth: 8,
}
}
}

/// Parse the command line arguments and return the list of files and formatting options.
///
/// # Arguments
Expand All @@ -70,31 +93,19 @@ pub struct FmtOptions {
///
/// A tuple containing a vector of file names and a `FmtOptions` struct.
#[allow(clippy::cognitive_complexity)]
#[allow(clippy::field_reassign_with_default)]
fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions)> {
// by default, goal is 93% of width
const DEFAULT_GOAL_TO_WIDTH_RATIO: usize = 93;

let matches = uu_app().try_get_matches_from(args)?;

let mut files: Vec<String> = matches
.get_many::<String>(ARG_FILES)
.map(|v| v.map(ToString::to_string).collect())
.unwrap_or_default();

let mut fmt_opts = FmtOptions {
crown: false,
tagged: false,
mail: false,
uniform: false,
quick: false,
split_only: false,
use_prefix: false,
prefix: String::new(),
xprefix: false,
use_anti_prefix: false,
anti_prefix: String::new(),
xanti_prefix: false,
width: 79,
goal: 74,
tabwidth: 8,
};
let mut fmt_opts = FmtOptions::default();

fmt_opts.tagged = matches.get_flag(OPT_TAGGED_PARAGRAPH);
if matches.get_flag(OPT_CROWN_MARGIN) {
Expand Down Expand Up @@ -141,7 +152,10 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions)
),
));
}
fmt_opts.goal = cmp::min(fmt_opts.width * 94 / 100, fmt_opts.width - 3);
fmt_opts.goal = cmp::min(
fmt_opts.width * DEFAULT_GOAL_TO_WIDTH_RATIO / 100,
fmt_opts.width - 3,
);
};

if let Some(s) = matches.get_one::<String>(OPT_GOAL) {
Expand All @@ -155,7 +169,10 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions)
}
};
if !matches.get_flag(OPT_WIDTH) {
Copy link
Collaborator

@tertsdiepraam tertsdiepraam May 31, 2023

Choose a reason for hiding this comment

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

Unrelated to your change, I'm getting a runtime error here because OPT_WIDTH is not marked as a flag in clap:

thread 'main' panicked at 'Mismatch between definition and access of `width`. Could not downcast to bool, need to downcast to alloc::string::String
', /home/terts/Programming/uutils/coreutils/src/uu/fmt/src/fmt.rs:171:21

fmt_opts.width = cmp::max(fmt_opts.goal * 100 / 94, fmt_opts.goal + 3);
fmt_opts.width = cmp::max(
Copy link
Collaborator

@tertsdiepraam tertsdiepraam May 31, 2023

Choose a reason for hiding this comment

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

This doesn't really have anything to do with your change, but are we sure this is right?

I also think the way this is implemented is a bit strange, maybe it would be better in a match expression:

let width_opt = matches.get_one::<String>(OPT_WIDTH);
let goal_opt = matches.get_one::<String>(OPT_GOAL);
match (width_opt, goal_opt) {
    (Some(width), Some(goal)) => {...},
    ...
}

Maybe we can make a good-first-issue for someone to clean this up a bit.

fmt_opts.goal * 100 / DEFAULT_GOAL_TO_WIDTH_RATIO,
fmt_opts.goal + 3,
);
} else if fmt_opts.goal > fmt_opts.width {
return Err(USimpleError::new(1, "GOAL cannot be greater than WIDTH."));
}
Expand Down Expand Up @@ -356,14 +373,14 @@ pub fn uu_app() -> Command {
Arg::new(OPT_WIDTH)
.short('w')
.long("width")
.help("Fill output lines up to a maximum of WIDTH columns, default 79.")
.help("Fill output lines up to a maximum of WIDTH columns, default 75.")
.value_name("WIDTH"),
)
.arg(
Arg::new(OPT_GOAL)
.short('g')
.long("goal")
.help("Goal width, default ~0.94*WIDTH. Must be less than WIDTH.")
.help("Goal width, default of 93% of WIDTH. Must be less than WIDTH.")
.value_name("GOAL"),
)
.arg(
Expand Down