Preserve newlines in Doc comments #2389

[asteding]

Hello,

I am no expert. But isn't this already the fix you'd like to have to close #2389 ?

If i run your example code

cargo run -- --help

I get the following output:

    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/playground --help`
xh 

USAGE:
    playground [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).

FLAGS:
    -h, --help
            Prints help information

    -V, --version
            Prints version information

[pksunkara]

Yeah looks like. You would need to get the CI to pass though.

[asteding]

Yeah, I'm gonna take care of it tomorrow. Just wanted a double check of the fix before I start working on the tests.
Thanks @pksunkara :)

[pksunkara]

Looking at the test case you fixed, I remembered why we didn't do it in the first place. Doc comments are generally parsed to only care about newlines when there are \n\n.

I think we might need a little bit of research and design first here before deciding on the implementation.

Can you please upload a picture of how cargo doc generates the doc comment the original code had?

[asteding]

Hello,
i am not sure what you mean with

Can you please upload a picture of how cargo doc generates the doc comment the original code had?

You mean what the output of cargo run -- --help to stdout?
Example 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();
}

Before with lines.iter().map(|s| s.trim()).collect::<Vec<_>>().join(" ")

    Finished dev [unoptimized + debuginfo] target(s) in 1.68s
     Running `target/debug/playground --help`
xh

USAGE:
    playground [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).

FLAGS:
    -h, --help
            Prints help information

    -V, --version
            Prints version information

After with lines.iter().map(|s| s.trim()).collect::<Vec<_>>().join('\n')

    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/playground --help`
xh

USAGE:
    playground [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).

FLAGS:
    -h, --help
            Prints help information

    -V, --version
            Prints version information

Is this what you requested?
Best regards
Andreas

[pksunkara]

No, what cargo doc --no-deps --open generates for that code. Once you run that and the browser is opened, I would like to see how the documentation for that field is printed in html.

[asteding]

Okay alright.
I did run cargo doc --no-deps --open that's the result.
https://paste.xinu.at/6PR5Lt/

Edit: Just double checked, and the change has no effect on the cargo doc command.

[pksunkara]

I think we need some kind of markdown to text converter library. As of right now, this would be a wrong implementation.

[asteding]

Right now I don't understand why this is a wrong implementation, wouldn't that mean that the code right now is lacking that feature aswell?

Because as i said i double checked the current code and the output of cargo doc with the fix and the current code is identical.

[pksunkara]

So, the doc comments are markdown. Which means two lines which are not separated by an empty line should be treated as a single line of text without any \n between them.

The issue now comes up when the lines are in list format * or -. And more issues might come up with other format like > etc. What we need to do is to recognize the lines correctly with this markdown format which is why I was talking about a converter. We probably need to add this behind a feature flag and that should be okay.

I think we also need to take notice of how textwrap dep might interact with these preserved lines. In fact, I am now thinking, this might be a good feature for textwrap itself rather than clap. @mgeisler What do you think?

[mgeisler]

I think we also need to take notice of how textwrap dep might interact with these preserved lines. In fact, I am now thinking, this might be a good feature for textwrap itself rather than clap. @mgeisler What do you think?

I'm not 100% up to speed this feature, but I would be happy to make textwrap useful for things like Markdown. Infact, I happen to have added a function in the last release which can re-wrap blocks of text in some simple cases: textwrap::refill. It's not a drop-in solution, but I think it could be a building block for one.

As for newlines, textwrap simply preserves them. So

println!("{}", textwrap::fill("foo bar baz...\nxxx yyy zzz...", width));

is the same as

println!("{}", textwrap::fill("foo bar baz...", width));
println!("{}", textwrap::fill("xxx yyy zzz...", width));

---

Now, you say that the docstrings are in Markdown format. Is this a requirement because of how they're handed to Clap by the thing that parses the Rust code and extracts the string? I would intuitively have assumed that the string was just passed as-in to Clap. If it is passed as-is, then I would argue that it should not be treated as Markdown since, well, the terminal isn't a Markdown interpreter.

However, even if the string is passed literally to Clap, I guess users will run into all sorts of problems with cargo doc if their project contain non-Markdown docstrings.

Related, I once made a little demo of word-wrapping Markdown text for the terminal. It sounds like this might do more or less what you ask: mgeisler/textwrap#140 (comment).

[pksunkara]

I might reply with more later, but we want to consider the docstrings as markdown when parsed by clap because we want to play nice with cargo doc.

[pksunkara]

Let's see some examples:

This is a paragraph that spans over two lines
but is just one line when the markdown is rendered.

If we send this to textwrap without removing the newlines:

This is a paragraph that spans over
two lines
but is just one line when the
markdown is rendered.

For lists:

* A good list item
* A list item that is actually so long that it is
broken into two lines
* A list item that has more elements in it

  This is a another paragraph that is also very long
such that it is broken over two lines.

Same issue if we don't remove newlines

* A good list item
* A list item that is actually so long
that it is
broken into two lines
* A list item that has more elements
in it

  This is a another paragraph that is
also very long
such that it is broken over two lines.

For blockquotes:

> This is a really long paragraph that should be
> broken in two lines.

When wrapped:

> This is a really long paragraph that
should be
> broken in two lines.

And I haven't even gone into lists in blockquotes, nested blockquotes etc.. (We don't need to support them because that's too much for a CLI tool)

Ideally, we can convert the markdown into text before sending to textwrap. But the blockquote scenario makes me think that having markdown care in textwrap is better.

[mgeisler]

Let me just say that I haven't forgotten about this, I'm just a little busy at the moment so please don't expect a lot of movement on this right now.

There might be better and existing solutions of you want full-blown Markdown in the terminal: https://github.com/Canop/termimad

I don't know the library yet, but it promise to do more or less what you want. I don't know if it can be used in a simple "here is some Markdown, please format as text" fashion or if it wants full control over the terminal.

I would also look at https://github.com/kivikakk/comrak, which is know can reformat Markdown to accommodate for different line widths — again similar to what you're trying to do here.

Just some notes for now... My feeling is that textwrap ought to be used by the higher level libraries above, not the other way around (maybe they already do, haven't checked yet).

[pksunkara]

I am going to close this PR because this needs more design.