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

Publish CLI help on docs website #2890

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Publish CLI help on docs website #2890

merged 3 commits into from
Jan 30, 2024

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Jan 28, 2024

This is the opposite of #1294. It was last requested on Discord.

I'm using a hacky approach that (ab)uses an insta to generate the Markdown. See more details in the commit descriptions (including a bunch of alternate approaches and why I didn't choose them). The insta approach ended up too tempting to me, in spite of its hackiness.

I have no control over what the Markdown looks like, since I don't believe the
library I'm currently using (https://docs.rs/clap-markdown/latest/clap_markdown/)
has any configuration. We could later switch to generating Markdown more manually,
it shouldn't be too hard. However, I find the library's output nice enough.

Here is an example HTML output to peruse:
generated-cli-reference.zip.

@ilyagr ilyagr force-pushed the cli-md-insta branch 2 times, most recently from 5fa3648 to a506d03 Compare January 28, 2024 05:44
@ilyagr ilyagr marked this pull request as ready for review January 28, 2024 05:45
@ilyagr ilyagr marked this pull request as draft January 28, 2024 05:47
@ilyagr ilyagr force-pushed the cli-md-insta branch 4 times, most recently from 96361d4 to 4b33760 Compare January 28, 2024 06:04
@ilyagr ilyagr marked this pull request as ready for review January 28, 2024 06:04
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

This LGTM even if the approach is hacky.

Possible alternatives

My next favorite approaches (which we could switch to later) would be:

xtask

Set up a CI check and a [Cargo xtask] so that cargo xtask cli-help-to-md
essentially runs cargo run -- util markdown-help > docs/cli-reference.md from
the project root.

Every developer would have to know to run cargo xtask cli-help-to-md if
they change the help text.

Eventually, we could have cargo xtask preflight that runs cargo +nightly fmt; cargo xtask cli-help-to-md; cargo nextest run, or cargo insta.

If we wanted to go the xtask or shell script way, I'd rather use a subset of "Blaze", be it Buck2 or just Bazel itself.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 30, 2024

Thanks Philip!

I'll merge this now, since it might require people to rebase and rerun cargo insta, and everybody will be rebasing anyway after the required checks changed due to #2895.

We can undo this and use another approach if it doesn't work well in practice.

If we wanted to go the xtask or shell script way, I'd rather use a subset of "Blaze", be it Buck2 or just Bazel itself.

I'm not sure I'm following your thoughts exactly, but using Bazel might not help very much with this, at least as long as we like having the docs stored in the repo (and thus changes showing in PR reviews).

I'm also a bit wary to make people install more tools (Rust + Bazel). The advantage of doing everything the Rust-y way is that people can easily jump into the project as they are learning Rust. But perhaps we'll switch someday, I don't know.

This uses the [`clap-markdown`] library. It's not very flexible, but seems to
work. Its implementation is not difficult. If needed, we could later
reimplement the part that iterates over all subcommands and have a different
output (e.g., the boring version of each help text inside its own code block,
or a different file per subcommand, or some fancy template in handlebars or
another rust-supported templating language). I don't want to do it right now,
though.

The library does turn out to have some annoying bugs, e.g.
ConnorGray/clap-markdown#18, but I think that's not a
deal-braker.  The fix seems to be 3 lines; if the fix doesn't get merged soon, we
could vendor the library maybe?

[`clap-markdown`]: https://docs.rs/clap-markdown/latest/clap_markdown/
I am using a very hacky approach, using `insta` to generate the markdown help.
This is based on a lucky coincidence that `insta` chose to use a header
format for snapshot files that MkDocs interprets as a Markdown header (and
ignores).

I considered several other approaches, but I couldn't resist the facts that:

- This doesn't require new developers to install any extra tools or run any
extra commands.
- There is no need for a new CI check.
- There is no need to compile `jj` in the "Make HTML docs" GitHub action,
  which is currently very fast and cheap.

Downside: I'm not sure how well MkDocs will work on Windows, unless the
developer explicitly enables symbolic links (which IIUC is not trivial).

### Possible alternatives 

My next favorite approaches (which we could switch to later) would be:

#### `xtask`

Set up a CI check and a  [Cargo `xtask`]  so that `cargo xtask cli-help-to-md`
essentially runs `cargo run -- util markdown-help > docs/cli-reference.md` from
the project root.

Every developer would have to know to run `cargo xtask cli-help-to-md` if
they change the help text.

Eventually, we could have `cargo xtask preflight` that runs `cargo +nightly
fmt; cargo xtask cli-help-to-md; cargo nextest run`, or `cargo insta`.

#### Only generate markdown for CLI help when building the website, don't track it in Git.

I think that having the file in the repo will be nice to preview changes to
docs, and it'll allow people to consult the file on GitHub or in their repo.

The (currently) very fast job of building the website would now require
installing Rust and building `jj`. 

#### Same as the `xtask`, but use a shell script instead of an `xtask`

An `xtask` might seem like overkill, since it's Rust instead of a shell script.
However, I don't want this to be a shell script so that new contributors on
Windows can still easily run it ( since this will be necessary for the CI to
pass) without us having to support a batch file.

#### Cargo Alias

My first attempt was to set up a [cargo alias] to run this, but that doesn't
support redirection (so I had to change the `jj util` command to output to a
file) and, worse, is incapable of executing the command *in the project root*
regardless of where in the project the current directory is. Again, this seems
to be too inconvenient for a command that every new PR author would have to run
to pass CI.

Overall, this just seems a bit ugly. I did file
rust-lang/cargo#13348, I'm not really sure that was
worthwhile, though.

**Aside:** For reference, the alias was:
    
```toml
# .cargo/config.toml
alias.gen-cli-reference = "run -p jj-cli -- util markdown-help docs/cli-reference.md"
```

### Non-alternatives 

#### Clap's new feature

`clap` recently obtained a similarly-sounding feature in
clap-rs/clap#5206. However, it only prints short help
for subcommands and can't be triggered by an option AFAICT, so it won't help us
too much.

[Cargo `xtask`]: https://github.com/matklad/cargo-xtask
[cargo alias]: https://doc.rust-lang.org/cargo/reference/config.html#alias
@ilyagr ilyagr enabled auto-merge (rebase) January 30, 2024 22:51
@ilyagr ilyagr merged commit 52c415e into jj-vcs:main Jan 30, 2024
15 checks passed
@ilyagr ilyagr deleted the cli-md-insta branch January 30, 2024 23:31
@PhilipMetzger
Copy link
Contributor

If we wanted to go the xtask or shell script way, I'd rather use a subset of "Blaze", be it Buck2 or just Bazel itself.

I'm not sure I'm following your thoughts exactly, but using Bazel might not help very much with this, at least as long as we like having the docs stored in the repo (and thus changes showing in PR reviews).

I'm also a bit wary to make people install more tools (Rust + Bazel). The advantage of doing everything the Rust-y way is that people can easily jump into the project as they are learning Rust. But perhaps we'll switch someday, I don't know.

Explaining myself a bit better. If we get to a stage where we (ab)use cargo anyway (cargo xtask) a definite build system would be better. This shouldn't mean that the cargo build is deprecated or removed, but since we have a PR for Buck2 anyway, it wouldn't be hard to bite the bullet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants