fmt: implement Default for FmtOptions #4747
Conversation
cakebaker
force-pushed
the
fmt_implement_default_for_fmtoptions
branch
from
f711d6d
to
7644a75
Compare
cakebaker
force-pushed
the
fmt_implement_default_for_fmtoptions
branch
from
7644a75
to
167ac6e
Compare
|
In the latest push I adapted the defaults to match those of GNU fmt. |
|
GNU testsuite comparison: |
src/uu/fmt/src/fmt.rs
Outdated
| @@ -140,7 +148,7 @@ 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 * 93 / 100, fmt_opts.width - 3); | |||
There was a problem hiding this comment.
would it be possible to move 93 into a const with a good variable name?
to make it more clear
There was a problem hiding this comment.
I hope it is more clear now.
cakebaker
force-pushed
the
fmt_implement_default_for_fmtoptions
branch
from
167ac6e
to
ec44577
Compare
|
GNU testsuite comparison: |
cakebaker
force-pushed
the
fmt_implement_default_for_fmtoptions
branch
from
ec44577
to
21d30a9
Compare
|
GNU testsuite comparison: |
| @@ -155,7 +169,10 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions) | |||
| } | |||
| }; | |||
| if !matches.get_flag(OPT_WIDTH) { | |||
| fmt_opts.width = cmp::max(fmt_opts.goal * 100 / 94, fmt_opts.goal + 3); | |||
| fmt_opts.width = cmp::max( | |||
There was a problem hiding this comment.
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.
| @@ -155,7 +169,10 @@ fn parse_arguments(args: impl uucore::Args) -> UResult<(Vec<String>, FmtOptions) | |||
| } | |||
| }; | |||
| if !matches.get_flag(OPT_WIDTH) { | |||
There was a problem hiding this comment.
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
21d30a9
to
09db8d8
Compare
|
GNU testsuite comparison: |
|
I already approved this, but consider this a second approval if the checks are now green (something was up in |
|
GNU testsuite comparison: |
This PR implements
DefaultforFmtOptions.I'm not sure whether the defaults for
widthandgoal, which is derived fromwidth, are correct. Or whether they differ from the ones used by GNU fmt on purpose. GNU fmt uses awidthof75as default, whereas we use79.