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.

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

Generated man pages still claim that flag options take values #4443

Closed
2 tasks done
bgilbert opened this issue Nov 2, 2022 · 8 comments · Fixed by #4645
Closed
2 tasks done

Generated man pages still claim that flag options take values #4443

bgilbert opened this issue Nov 2, 2022 · 8 comments · Fixed by #4645
Labels
A-man Area: man generator C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@bgilbert
Copy link
Contributor

bgilbert commented Nov 2, 2022

Please complete the following tasks

Rust Version

rustc 1.64.0 (Fedora 1.64.0-1.fc36)

Clap Version

4.0.18, clap_mangen 0.2.4

Minimal reproducible code

use clap::{CommandFactory, Parser};

#[derive(Parser)]
/// Command to do things
pub struct Cmd {
    /// Do thing
    #[clap(long)]
    thing: bool,
}

fn main() {
    Cmd::parse();
    clap_mangen::Man::new(Cmd::command())
        .render(&mut std::io::stdout())
        .unwrap();
}

Cargo.toml:

[package]
name = "clap-man"
version = "0.0.0"
edition = "2021"

[dependencies]
clap = { version = "4", features = ["derive"] }
clap_mangen = "0.2"

Steps to reproduce the bug with the above code

cargo run | man -l -

Actual Behaviour

The option description for --thing claims that it takes an argument:

clap-man(1)                 General Commands Manual                clap-man(1)

NAME
       clap-man - Command to do things

SYNOPSIS
       clap-man [--thing] [-h|--help]

DESCRIPTION
       Command to do things

OPTIONS
       --thing=THING
              Do thing

       -h, --help
              Print help information

                                   clap-man                        clap-man(1)

It doesn't:

$ cargo run -- --thing=true
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
     Running `target/debug/clap-man --thing=true`
error: The value 'true' was provided to '--thing' but it wasn't expecting any more values

Usage: clap-man --thing

For more information try '--help'

Expected Behaviour

The man page lists the option without any argument, as it did in clap 3.2 / clap_mangen 0.1:

clap-man(1)                 General Commands Manual                clap-man(1)

NAME
       clap-man - Command to do things

SYNOPSIS
       clap-man [--thing] [-h|--help]

DESCRIPTION
       Command to do things

OPTIONS
       --thing
              Do thing

       -h, --help
              Print help information

                                   clap-man                        clap-man(1)

Additional Context

This is a followup to #4410. clap_mangen 0.2.4 improved the situation but didn't fix it.

Debug Output

No response

@bgilbert bgilbert added the C-bug Category: Updating dependencies label Nov 2, 2022
@epage epage added E-easy Call for participation: Experience needed to fix: Easy / not much A-man Area: man generator E-help-wanted Call for participation: Help is requested to fix this issue. labels Nov 2, 2022
Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Nov 23, 2022
The man generation would generate suggestions for all options of
values they should take, even if the argument did not take values.
This adds a simple check to make sure only options that take values
offer a suggestion

fixes: clap-rs#4443
Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Nov 23, 2022
The man generation would generate suggestions for all options of
values they should take, even if the argument did not take values.
This adds a simple check to make sure only options that take values
offer a suggestion

fixes: clap-rs#4443
@epage
Copy link
Member

epage commented Nov 25, 2022

As pointed out in #4506, #4432 highlights output where this is working and we have tests highlighting this working.

@Calder-Ty pointed out regarding #4506

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

@Calder-Ty
Copy link
Contributor

I want to clarify that it is the proposed change in #4506 that brought a fix for the given examples above. However as pointed out, in the PR the change doesn't affect existing tests, which we should expect it to. I'm still trying to dig up what changed between #4432 and now, as i'm able to replicate the issue in the report.

@epage
Copy link
Member

epage commented Nov 25, 2022

The one big difference I notice this Issue is using the derive API and the existing tests are using the builder API. Since #4506 fixes this, it implies that a value name is being set for flags in the derive.

It looks like we always set value_name in the derive: https://github.com/clap-rs/clap/blob/master/clap_derive/src/derives/args.rs#L232

Normally, we verify the relationship between num_args and value_names but apparently we skip it for empty value names to make the derive easier: https://github.com/clap-rs/clap/blob/master/src/builder/debug_asserts.rs#L757

So the question is if we should fix this in the derive or in the callers.

I will point out that clap_mangen is a bit simple in its rendering of values; --help has a bit more complex of rendering: https://github.com/clap-rs/clap/blob/master/src/builder/arg.rs#L4148

@epage
Copy link
Member

epage commented Nov 25, 2022

One option would be for us to expose an Arg::render function that calls Arg::stylized(None). The only public way to access the styling is with StyledStr::ansi().to_string() and it will remain that way. I've been considering writing an ansi code to roff converter.

If someone wants to step up and write the ansi to roff converter as I won't be getting to it immediately, that'd be great. I have notes to help prepare for it in the issues at https://github.com/epage/anstyle/issues.

However, that is a big ask and people likely don't want to wait that long. I can understand just duplicating the code into clap_mangen until then.

@Calder-Ty
Copy link
Contributor

@epage, I don't mind working getting a workaround you describe setup for now. I don't know if i'm up to the ANSI conversion, would have to look into that more.

Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Nov 30, 2022
It was noticed that between clap-rs#4443 and clap-rs#4432, an issue in the behavior
was that the derive api handles tasks with values slightly differently
than the declarative api. Added a test to show parity between
declaritive and derive api.
Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Nov 30, 2022
It was noticed that between clap-rs#4443 and clap-rs#4432, an issue in the behavior
was that the derive api handles tasks with values slightly differently
than the declarative api. Added a test to show parity between
declaritive and derive api.
Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Nov 30, 2022
It was noticed that between clap-rs#4443 and clap-rs#4432, an issue in the behavior
was that the derive api handles tasks with values slightly differently
than the declarative api. Added a test to show parity between
declaritive and derive api.
Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Dec 16, 2022
It was noticed that between clap-rs#4443 and clap-rs#4432, an issue in the behavior
was that the derive api handles tasks with values slightly differently
than the declarative api. Added a test to show parity between
declaritive and derive api.
Calder-Ty added a commit to Calder-Ty/clap that referenced this issue Dec 16, 2022
It was noticed that between clap-rs#4443 and clap-rs#4432, an issue in the behavior
was that the derive api handles tasks with values slightly differently
than the declarative api. Added a test to show parity between
declaritive and derive api.
@Calder-Ty
Copy link
Contributor

@epage Sorry for leaving this alone for so long. I've been looking into ways to make a quick workaround
for this in a way that is robust. After looking into it I think the best solution is just to go forwards
with creating the ANSI-Roff converter.

Adding an Arg::Render function is feasible, but it also generates styles that would be noticeably different
from the current manpage styling. I'm not sure how "in stone" those need to be, but want to err on
the conservative side.

Copying the code into clap_mangen is a lot of reduplication, as there are a lot of private dependencies
that would need to be copied over as well.


I wouldn't mind working on writing an ANSI to Roff converter, but I would like to have
a better idea of what you are imagining that looking like, perhaps in another discussion. For
now I have added some tests to #4506 that demonstrate some parity between Derive and Builder commands,
and that they both show flag options appropriately.

As an aside, when testing these different API's I found some other cosmetic disparities between
Derive and Builder API's that I will write up another ticket for. However it probably would be better
to get the ansi-roff converter done first as that will probably clear up the differences.

@epage
Copy link
Member

epage commented Dec 17, 2022

I wouldn't mind working on writing an ANSI to Roff converter, but I would like to have
a better idea of what you are imagining that looking like

In https://github.com/epage/anstyle/issues/4, I contrast different ANSI parsing libraries. For a first round, feel free to use what is easiest as long as it as a good license. I'm less concerned about clap_mangens build times. We can then iterate on it as clap needs to adopt an ANSI parser.

anstyle is where I'm housing these different integrations, e.g. you can see what it supports today in the README.

Adding an Arg::Render function is feasible, but it also generates styles that would be noticeably different
from the current manpage styling. I'm not sure how "in stone" those need to be, but want to err on
the conservative side.

clap_mangen is in its very early stages; nothing is set in stone. I wasn't going to get started on it until we could reuse more help generation logic but some others were wanting it sooner and wrote most of the code. It still has a ways to go.

@bgilbert
Copy link
Contributor Author

Pending the broader refactor, I've submitted a localized fix in #4645.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-man Area: man generator C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
3 participants