man page rendering is broken, resulting in broken examples

[vassudanagunta]

broken examples on man page as rendered in bash on macOS:

Regex use. Let's trim some trailing whitespace
       $ echo 'lorem ipsum 23   ' | sd '' ''
       lorem ipsum 23

Indexed capture groups
       $ echo 'cargo +nightly watch' | sd '(0(24
       cmd: cargo, channel: nightly, subcmd: watch

Named capture groups
       $ echo "123.45" | sd '(?P<dollars>+).(?P<cents>+)' '$dollars dollars and $cents cents'
       123 dollars and 45 cents

Same examples from README:

2. Basic regex use - let's trim some trailing whitespace

> echo 'lorem ipsum 23   ' | sd '\s+$' ''
lorem ipsum 23

3. Capture groups

Indexed capture groups:

> echo 'cargo +nightly watch' | sd '(\w+)\s+\+(\w+)\s+(\w+)' 'cmd: > $1, channel: $2, subcmd: $3'
cmd: cargo, channel: nightly, subcmd: watch

Named capture groups:

> echo "123.45" | sd '(?P<dollars>\d+)\.(?P<cents>\d+)' '$dollars > dollars and $cents cents'
123 dollars and 45 cents

[vassudanagunta]

I would fix it but I know neither Rust or anything about man page generation.

If no one has the time to figure it out, then I'd strongly at least removing those examples, as they do zero good leaving them as they are.

[alerque]

Since this project already uses clap for the CLI arg parsing it would make sense to drop both the custom completions and the man page code from the repository and have it generated at build time by clap_complete and clap_mangen respectively. These crates can be triggered just from build.rs so they don't have to be baked into the final binary but can generate fresh up to date completions and man pages at build time so they never get out of sync. This is probably something I could contribute if the project is willing to accept it. Additionally if the project is interested I could add autotools plumbing so that all the unix-y bits like completions and man pages would get installed to the correct places if installed with ./configure && make install. This would not hinder installation of the bare binary with cargo install, but for those that chose to use the autotools build method would result in not just the binary but the supporting files as well being installed to the correct places. Far an example of this being done (completions, man pages, & autotools install) you can see my git-warp-time project.

[CosmicHorrorDev]

Since this project already uses clap for the CLI arg parsing it would make sense to drop both the custom completions and the man page code from the repository and have it generated at build time by clap_complete and clap_mangen respectively. These crates can be triggered just from build.rs so they don't have to be baked into the final binary but can generate fresh up to date completions and man pages at build time so they never get out of sync

Completions and the manpage are both programmatically generated already using a cargo-xtask task (cargo-xtask is akin to make, but you're programming in Rust). Running

$ cargo xtask gen

from within the repository generates all the completions as well as the manpage using the clap_complete and man crates respectively. Running this command is part of the steps to follow for cutting a release, so they'll never get too stale

Additionally if the project is interested I could add autotools plumbing so that all the unix-y bits like completions and man pages would get installed to the correct places if installed with ./configure && make install.

I'm not familiar with autotools, but I could be open to PRs adding support

[alerque]

Okay I missed the xtask stuff. I saw the pre-built completions and man page and the complaints that they were out of sync, looked at Cargo.toml and saw clap but not any of the generators, and assumed they were just static.

As a general rule I would still say the generated artifacts shouldn't be tracked in Git at all. A generated source tarball could include them (ala how autotools generates a sources tarball via make dist with some generated stuff included for convenience).

I'll have a look and see if I can mix in autotools stuff without to much trouble so that the pregenerated artifacts can be dropped from Git while still letting people install the current ways plus being able to do a full install with supporting files.

[CosmicHorrorDev]

I'm totally content with the current xtask stuff. I don't see any compelling reason to drop it at the moment. Using an xtask is even mentioned as one of the means of generating completions in clap_complete's docs

From: https://docs.rs/clap_complete/latest/clap_complete/generator/fn.generate_to.html

NOTE: to generate the file at compile time you must use a build.rs “Build Script” or a cargo-xtask

[alerque]

Generating them works fine. What about installing them? Under the current system that is an exercise left up to the reader, no? And also it (as evidenced by multiple duplicate issues on this topic) having generated artifacts in the Git repository that are some measure of out of date is confusing to end users, wouldn't you agree? And possibly/probably to distro packages as well. How many distros have up 100% to date man pages and completions installed by default right now?

[nc7s]

Please, keep away from those "dox"/"xdo" things, unless with a good reason. They are non-standard and add burden to packagers (additional dependencies, manually maintained build process). The only "xtask" present can be easily done in build.rs. Also, you may forget to run it when publishing a release, but AFAIK cargo publish always runs build.rs.

As for checking in generated artifacts, as long as there is some measure to make sure updated source material comes with updated artifacts, e.g. using a git hook, it should be fine.

[CosmicHorrorDev]

What about installing them? Under the current system that is an exercise left up to the reader, no?

Which is why I said I could be open to PRs adding support for autotools. I assume you can install the pre-generated files using it

And also it (as evidenced by multiple duplicate issues on this topic) having generated artifacts in the Git repository that are some measure of out of date is confusing to end users, wouldn't you agree?

The issues you're referring to here actually predate us using a xtask back to when this repo used a build.rs file. They are due to the current usage of the man crate which requires keeping a bunch of strings up to date. Switching to clap_mangen would fix that, but it's not at all related to the current xtask usage

Them getting out of sync could certainly be confusing, but that would only happen between releases, and I'm planning on keeping that window much much tighter after this next release

And possibly/probably to distro packages as well. How many distros have up 100% to date man pages and completions installed by default right now?

I'm not sure how this is relevant to distros. I don't know of any distro that is shipping the current master of sd. If they are then that's unsupported (which I think is more than reasonable)

[CosmicHorrorDev]

@bnoctis

They are non-standard and add burden to packagers (additional dependencies, manually maintained build process).

There are no extra dependencies for packagers to handle, and the build process requires no more work than using a build script would

Also, you may forget to run it when publishing a release, but AFAIK cargo publish always runs build.rs.

As already mentioned re-generating the static assets is detailed in the release checklist. I won't forget to do it, although I understand the concern there :)

[nc7s]

They are non-standard and add burden to packagers (additional dependencies, manually maintained build process).

There are no extra dependencies for packagers to handle, and the build process requires no more work than using a build script would

Hmm, I mistook the "cargo-xtask" repository as a binary crate. Seems to be simply a methodology. That's better in terms of dependencies. My argument is then merely a preference: "free form task run" is too much for most, if not all, projects.

Also, you may forget to run it when publishing a release, but AFAIK cargo publish always runs build.rs.

As already mentioned re-generating the static assets is detailed in the release checklist. I won't forget to do it, although I understand the concern there :)

You won't, but if someone else becomes a maintainer and…? ;)

Anyway, let's fix the man page. I'll try to go with clap_mangen if you are not already doing that.

[CosmicHorrorDev]

You won't, but if someone else becomes a maintainer and…? ;)

Then they can decide when they take over :D

Anyway, let's fix the man page. I'll try to go with clap_mangen if you are not already doing that.

Sure! Feel free to open a PR and/or issue

[nc7s]

Note that the separate crate (xtask) disables direct import of Options and reading of Cargo.toml. Now those metadata have to be manually read and set.

[CosmicHorrorDev]

Can you elaborate on what you're talking about? I'm not sure what metadata reading/setting that you're doing

[nc7s]

You already include!ed ../../src/cli.rs, that's one thing. But clap reads the current Cargo.toml, which has the name xtask, so you gotta also cmd.name("sd").

Reading (the top level) Cargo.toml is required to get the version.

(Sorry my hand went faster than brain)

[CosmicHorrorDev]

Gotcha! You should be able to set name = "sd" in cli.rs and then that can be shared with the completions as well

As for the version - we can just keep sd's and the xtask versions in sync by setting it in the workspace Cargo.toml and then we can use version.workspace = true for both the workspace member crates