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

Clap derive should parse markdown doc comment into normal text #2389

Open
2 tasks done
ducaale opened this issue Mar 6, 2021 · 11 comments · May be fixed by #4444
Open
2 tasks done

Clap derive should parse markdown doc comment into normal text #2389

ducaale opened this issue Mar 6, 2021 · 11 comments · May be fixed by #4444
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate S-blocked Status: Blocked on something else such as an RFC or other implementation work.

Comments

@ducaale
Copy link
Contributor

ducaale commented Mar 6, 2021

Maintainer's notes:

  • Workaround: verbatim_doc_comment attribute

Blocked on finalizing details of StyledStr, including


Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.48.0 (7eac88abb 2020-11-16)

Clap Version

3.0.0-beta.2

Minimal reproducible code

use clap::Clap;

#[derive(Clap, Debug)]
#[clap(name = "xh")]
struct Cli {
    /// Optional key-value pairs to be included in the request.
    ///
    /// - key:=value to add a complex JSON value (e.g. `numbers:=[1,2,3]`)
    /// - key@filename to upload a file from filename (with --form)
    /// - header:value to add a header
    /// - header: to unset a header
    /// - header; to add a header with an empty value
    ///
    /// A backslash can be used to escape special characters (e.g. weird\:key=value).
    #[clap(value_name = "REQUEST_ITEM")]
    raw_rest_args: Vec<String>
}

fn main() {
      let _ = Cli::parse();
}

Steps to reproduce the bug with the above code

cargo run -- --help

Actual Behaviour

ARGS:
    <REQUEST_ITEM>...
            Optional key-value pairs to be included in the request.

            * key==value to add a parameter to the URL \n * key=value to add a JSON field (--json)
            or form field (--form) * key:=value to add a complex JSON value (e.g.
            `numbers:=[1,2,3]`) * key@filename to upload a file from filename (with --form) *
            header:value to add a header * header: to unset a header * header; to add a header with
            an empty value

            A backslash can be used to escape special characters (e.g. weird\:key=value).

Expected Behaviour

ARGS:
    <REQUEST_ITEM>...
            Optional key-value pairs to be included in the request.

            * key==value to add a parameter to the URL
            * key=value to add a JSON field (--json) or form field (--form)
            * key:=value to add a complex JSON value (e.g. `numbers:=[1,2,3]`)
            * key@filename to upload a file from filename (with --form)
            * header:value to add a header
            * header: to unset a header
            * header; to add a header with an empty value

            A backslash can be used to escape special characters (e.g. weird\:key=value).

Additional Context

In StructOpt, it possible to use the long_help attribute to preserve newlines but I couldn't find something similar in clap-derive.

Just found that it is called long_about in clap-derive.

Debug Output

Will add if requested

@ducaale ducaale added the C-bug Category: Updating dependencies label Mar 6, 2021
@pksunkara pksunkara added A-derive Area: #[derive]` macro API D: easy 💸 $5 labels Mar 6, 2021
@pksunkara pksunkara added this to the 3.0 milestone Mar 6, 2021
asteding pushed a commit to asteding/clap that referenced this issue Mar 9, 2021
@pksunkara
Copy link
Member

There is quite a bit of discussion in #2401

@pksunkara pksunkara modified the milestones: 3.0, 3.1 Apr 23, 2021
@pksunkara pksunkara changed the title Preserve newlines in Doc comments Clap derive should parse markdown doc comment into normal text Nov 14, 2021
@epage epage removed this from the 3.1 milestone Dec 13, 2021
@epage epage added the S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing label Dec 13, 2021
@epage
Copy link
Member

epage commented Dec 13, 2021

One of the challenges with this is we'd basically need to pull in pulldown-cmark which is quite a large dependency. This would need to be put behind a feature flag.

@ducaale
Copy link
Contributor Author

ducaale commented Dec 13, 2021

My main goal was to preserve newlines in the doc comment and that should be achievable with the verbatim_doc_comment attribute.

Example

// clap = { version = "3.0.0-rc.4", features = ["derive"] }

use clap::Parser;

#[derive(Parser, Debug)]
#[clap(name = "xh")]
struct Cli {
    /// Optional key-value pairs to be included in the request.
    ///
    ///   • key:=value to add a complex JSON value (e.g. `numbers:=[1,2,3]`)
    ///   • key@filename to upload a file from filename (with --form)
    ///   • header:value to add a header
    ///   • header: to unset a header
    ///   • header; to add a header with an empty value
    ///
    /// A backslash can be used to escape special characters (e.g. weird\:key=value).
    #[clap(value_name = "REQUEST_ITEM", verbatim_doc_comment)]
    raw_rest_args: Vec<String>,
}

fn main() {
    let _ = Cli::parse();
}

Output

USAGE:
    temp-clap [REQUEST_ITEM]...

ARGS:
    <REQUEST_ITEM>...
            Optional key-value pairs to be included in the request.

              • key:=value to add a complex JSON value (e.g. `numbers:=[1,2,3]`)
              • key@filename to upload a file from filename (with --form)
              • header:value to add a header
              • header: to unset a header
              • header; to add a header with an empty value

            A backslash can be used to escape special characters (e.g. weird\:key=value).

OPTIONS:
    -h, --help
            Print help information

I wouldn't mind if this issue was to be closed.

@epage
Copy link
Member

epage commented Dec 13, 2021

I think its still an interesting topic for us to weigh out. I did call out the workaround in the issue so its more discoverable.

@I60R
Copy link

I60R commented Dec 29, 2021

We may need only a subset of markdown features in CLI parser:

  • newline escaping (implemented in Allow to escape newline with \ like in markdown #3228)
  • lists as suggested here (should be relatively easy to implement, even with nesting)
  • bold, italic, bold-italic, inline code - if colors enabled (not sure whether it's worth effort, but why not?)
  • fenced code blocks (without syntax highlighting, of course)

There also could be some mechanism to cut off help message from doc comment as @epage suggested here — IMO markdown headers are a perfect mechanism for that e.g.:

  • without any header everything goes to -h --help output
    • that's the same as if everything was placed under # Help header
  • if # Long help is specified that section will be appended to --help output (only)
  • we may also have # Manpage section (if clap will provide some mechanism for manpage generation)
  • text under all other headers will be ignored by clap

I think this subset should satisfy 99.9% of all use cases of clap, everyone should like it, and it doesn't seem to be so complicated to require pulldown-cmark or any other heavy dependency.

@epage
Copy link
Member

epage commented Dec 29, 2021

My experience writing one off parsers has made me a bit cautious of doing so. I threw together some md parser benchmarks to see what might be small enough for being an optional dependency. minimad looks tempting.

@epage
Copy link
Member

epage commented Dec 30, 2021

Another thought on markdown support, should we support it at runtime, compile time, or both?

The core of this Issue is our line breaks which is a problem exclusive to the derive API. For that, we could build markdown parsing directly into clap_derive.

Another step up is markdown formatting. For that, we'd want to expose it to both APIs. Say we generalized our Colorizer struct and accepted it for all our user-facing inputs (for #1790, #1433), users can do whatever styling they want, though it might be a bit arduous. We then provided a markdown-parsing macro that would code-generate Colorizer. clap_derive would then have this built-in for all doc comments.

The main benefit of this is that this would remove the binary-size overhead of the markdown parser. We'd still need to compile it (though it'd be optional). Though this would mean we wouldn't regress in --help and error performance by introducing markdown, the performance would have to be pretty bad for us to care.

This has the added benefit that we wouldn't need yet-another Setting in the API for doing controlling this at runtime.

@epage
Copy link
Member

epage commented Dec 30, 2021

Another question for us to answer in this issue is how disruptive would markdown parsing be. While --helps output is not considered "stable", we shouldn't dramatically alter users crafting of their help output. To what degree of markdown parsing can we add in a patch, a feature release, or a breaking release?

epage added a commit to epage/clap that referenced this issue Dec 30, 2021
This reverts commit 333b993.

PR clap-rs#1810's motivation was effectively "this is redundant with `\n`".
That is a fine motivation.  Unfortunately, we don't have a way to force
a hard break in `clap_derive`.  clap-rs#2389 should help with this eventually
but we shouldn't hold up 3.0 to get that ready.  So in the mean time, we
are restoring `{n}`.

We have clap-rs#3230 for tracking the re-removal of it.
@gibfahn
Copy link
Contributor

gibfahn commented Dec 30, 2021

Another thought on markdown support, should we support it at runtime, compile time, or both?

Compile time only sounds reasonable to me (but I only use the derive API, so I guess I would say that 😁) .

Another question for us to answer in this issue is how disruptive would markdown parsing be.

I think that (as a user) if markdown parsing were turned on for existing help text that would probably break a bunch of people, but if it was behind a flag, or minor updates to the markdown output were later made in minor/patch updates, that would probably be fine, as what was intended to be output would still be there, just maybe looking slightly prettier.

The core of this Issue is our line breaks which is a problem exclusive to the derive API. For that, we could build markdown parsing directly into clap_derive.

For what it's worth there are a bunch of other things I would use if we had markdown support, not least:

  1. Italic and bold inline code
  2. Subheadings and code blocks, e.g.:
    ## EXAMPLES: <!-- Should be highlighted yellow the same way the other sections in the generated help like USAGE: are today. -->
    
    <!-- Syntax highlighting in examples would be really neat. -->
    ```console
    # Run the subcommand with the arg specified.my-command my-subcommand --my-arg
    ```
  3. docs.rs compatibility -> If you have a binary/library combo that takes a set of options, it's nice to be able to use the same documentation for docs.rs and for mycmd --help.

@epage
Copy link
Member

epage commented Dec 30, 2021

<!-- Syntax highlighting in examples would be really neat. -->

I was hoping someone wasn't going to say that :)

syntect is the main go-to for syntax highlighting. It can be a bit slow to compile and big. It can be made optional. Looks like we can also disable the built-in language / theme set. I doubt we need all of them. The question is what languages do we need at minimum and how should we respond if someone asks for another language.

We'll need to do some analysis to decide how worth it this is vs generating help at build-time

@epage
Copy link
Member

epage commented Jul 17, 2023

One design problem I've been running into is how to render markdown at compile time when the colors aren't known until runtime.

I think the way to do this is to allow a fn(fmt, palette) -> fmt::Result to be passed into StyledStr and the derive would generate this, parsing the markdown at compile time but rendering the styles at runtime.

@epage epage added E-medium Call for participation: Experience needed to fix: Medium / intermediate S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
5 participants