Skip to content
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

Jump to bottom

Mangen no longer generates suggestions for options that don't take values #4506

Closed
wants to merge 2 commits into from
Closed

Mangen no longer generates suggestions for options that don't take values #4506

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0f6289
fix: Don't suggest values for boolean options
Calder-Ty Nov 23, 2022
d9e5473
test: Add test of derive functionality
Calder-Ty Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions clap_mangen/src/render.rs
View file
Open in desktop
Expand Up @@ -99,9 +99,11 @@ pub(crate) fn options(roff: &mut Roff, cmd: &clap::Command) {
(None, None) => vec![],
};

if let Some(value) = &opt.get_value_names() {
header.push(roman("="));
header.push(italic(&value.join(" ")));
if opt.get_action().takes_values() {
Copy link
Member

Choose a reason for hiding this comment

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

This seems suspicious. #4432 showed working man pages. We should know what the difference is between the old issue and new issue and discuss it in this PRs issue for which route to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Yeah i'll look at that.

if let Some(value) = &opt.get_value_names() {
header.push(roman("="));
header.push(italic(&value.join(" ")));
}
}

if let Some(defs) = option_default_values(opt) {
Expand Down
14 changes: 14 additions & 0 deletions clap_mangen/tests/common.rs
View file
Open in desktop
Expand Up @@ -312,3 +312,17 @@ pub fn possible_values_command(name: &'static str) -> clap::Command {
]),
)
}

/// Checks to make sure boolean valued "Flag options" do not generate
/// suggestions for a parameter. i.e:
/// --boolean_flag=BOOLEAN_FLAG
///
/// This is both confusing and suggest erroneous behavior as clap will fail if you
/// pass a value to a boolean flag
pub fn flag_without_value(name: &'static str) -> clap::Command {
clap::Command::new(name).arg(
clap::Arg::new("is_bool")
.long("is_bool")
.action(clap::ArgAction::SetTrue),
)
}
7 changes: 7 additions & 0 deletions clap_mangen/tests/roff.rs
View file
Open in desktop
Expand Up @@ -70,6 +70,13 @@ fn possible_values() {
common::assert_matches_path("tests/snapshots/possible_values.bash.roff", cmd);
}

#[test]
fn flag_without_value() {
Copy link
Member

Choose a reason for hiding this comment

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

Did you verify the behavior of the test changed with clab_mangen changes? I see other tests using SetTrue but their snapshots didn't change, implying this PR didn't have an affect (see also my other comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I see i was being too naive with the solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's strange because the change definitely does work on the example provided in the bug report

let name = "my-app";
let cmd = common::flag_without_value(name);
common::assert_matches_path("tests/snapshots/flag_without_value.bash.roff", cmd);
}

#[test]
fn sub_subcommands_help() {
let name = "my-app";
Expand Down
15 changes: 15 additions & 0 deletions clap_mangen/tests/snapshots/flag_without_value.bash.roff
View file
Open in desktop
@@ -0,0 +1,15 @@
.ie /n(.g .ds Aq /(aq
.el .ds Aq '
.TH my-app 1 "my-app "
.SH NAME
my/-app
.SH SYNOPSIS
/fBmy/-app/fR [/fB/-/-is_bool/fR] [/fB/-h/fR|/fB/-/-help/fR]
.SH DESCRIPTION
.SH OPTIONS
.TP
/fB/-/-is_bool/fR

.TP
/fB/-h/fR, /fB/-/-help/fR
Print help information