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

[documentation] #3509: Generate Kagami CLI help as Markdown #3510

Closed
wants to merge 7 commits into from

Conversation

0x009922
Copy link
Contributor

@0x009922 0x009922 commented May 22, 2023

Description

Self-explanatory PR title.

Linked issue

Closes #3509

Benefits

Same documentation (from doccomments) for both CLI and GitHub.

Checklist

  • Self-review
  • Merge [feature] #2373: kagami swarm #3475
  • Doc review
  • Initial code review
  • Fix comments
  • Re-review
  • Decide whether we should proceed with clap-markdown or with a self-made tool
  • Create a follow-up issue: generate kagami help in CI

@0x009922 0x009922 added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST Documentation Documentation changes labels May 22, 2023
@0x009922 0x009922 self-assigned this May 22, 2023
@0x009922 0x009922 mentioned this pull request May 22, 2023
5 tasks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the format that clap-markdown produces for a couple of reasons:

  • Mix of ## and #####
  • Using bold in headers: ##### **Options:**
  • Inconsistent spacing
  • character

Not critical though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, it ignores additional text in arguments doccomments and only includes the first line.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by this? From what I see, the doccomments are included in full (e.g. for genesis or crypto). Or did you mean that it doesn't preserve formatting without the verbatim attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0x009922
Copy link
Contributor Author

It is not much code to write a better docgen implementation. It might be used for other CLIs as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by this? From what I see, the doccomments are included in full (e.g. for genesis or crypto). Or did you mean that it doesn't preserve formatting without the verbatim attribute?

Comment on lines 220 to 221
* `client` —
* `peer` —
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have short descriptions here as well. Could be done by carrying over the old examples directly into the Args::Config docstring, just like you did with Args::Genesis, I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also would prefer some different title, as either the old README.md or maybe kagami.md sound more descriptive to me. And maybe have this markdown generation a part of our CI, just like e.g. schema is (could be a separate small PR though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely should be a part of CI.

Comment on lines 245 to 249
## `kagami tokens`

Generate a list of predefined permission tokens and their parameters

**Usage:** `kagami tokens`
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment about Args::Config also applies here, despite this part being insubstantial. Even though it's just two small and obvious examples there, I don't think we lose much by omitting them, but being more explicit never hurts.

tools/kagami/src/main.rs Outdated Show resolved Hide resolved
@ilchu
Copy link
Contributor

ilchu commented May 30, 2023

It is not much code to write a better docgen implementation. It might be used for other CLIs as well.

Been thinking about this too, you mean ditching the whole Documented trait and doing something similar for config endpoint docs?

@0x009922
Copy link
Contributor Author

It is not much code to write a better docgen implementation. It might be used for other CLIs as well.

Been thinking about this too, you mean ditching the whole Documented trait and doing something similar for config endpoint docs?

@ilchu,

Hmm, as far as I understand you are speaking about Iroha configuration documentation, which is generated by kagami docs. I am speaking not about it. (Although you are welcome to put your thoughts here: #3507)

Here I mean that clap-markdown implementation is not complex, and we can write our own clap-based markdown generator (e.g. iroha_clap_markdown crate), with a format that we consider a better option.

Comment on lines 290 to 297
* `--image <IMAGE>` — Use specified docker image
* `--build <PATH>` — Use local path location of the Iroha source code to build images from
* `--build-from-github` — Clone `hyperledger/iroha` repo from the revision Kagami is built itself, and use the cloned source code to build images from
* `-p`, `--peers <PEERS>` — How many peers to generate within the docker-compose
* `--outdir <OUTDIR>` — Target directory where to place generated files
* `--outdir-force` — Re-create the target directory if it already exists
* `--no-default-configuration` — Do not create default configuration in the `<outdir>/config` directory
* `-s`, `--seed <SEED>` — Might be useful for deterministic key generation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the example of that clap-markdown doesn't render additional documentation for arguments. Even if I put #[clap(verbatim_doc_comment)].

For example, --no-default-configuration's full documentation is as follows:

Do not create default configuration in the <outdir>/config directory.

Default config.json, genesis.json and validator.wasm are generated and put into
the <outdir>/config directory. That directory is specified in the Docker Compose
volumes field.

If you don't need the defaults, you could set this flag. The config directory will be
created anyway, but you should put the necessary configuration there by yourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I see it, thanks. So it doesn't recurse into the submodules args fully. That should be trivially fixable with a home-brew implementation (and sounds really close to what Documented does already anyways).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely should be a part of CI.

0x009922 and others added 4 commits May 31, 2023 07:40
Co-authored-by: Ilia Churin <[email protected]>
Signed-off-by: 0x009922 <[email protected]>
…into 3509-kagami-clap-markdown

# Conflicts:
#	tools/kagami/src/main.rs
@ilchu
Copy link
Contributor

ilchu commented May 31, 2023

@ilchu,

Hmm, as far as I understand you are speaking about Iroha configuration documentation, which is generated by kagami docs. I am speaking not about it. (Although you are welcome to put your thoughts here: #3507)

That's indeed what I was talking about. Then I'll see if I actually can come up with anything actionable up there!

@QuentinI
Copy link
Contributor

I'm a bit bothered that running subcommands with --help (e.g. kagami crypto --help) will output a wall of raw Markdown. I'm not sure the longer docs is even useful there as opposed to keeping it in documentation only; and of course unprocessed Markdown looks quite jarring in a help message

@QuentinI QuentinI self-assigned this May 31, 2023
@0x009922
Copy link
Contributor Author

0x009922 commented Jun 6, 2023

I'm a bit bothered that running subcommands with --help (e.g. kagami crypto --help) will output a wall of raw Markdown. I'm not sure the longer docs is even useful there as opposed to keeping it in documentation only; and of course unprocessed Markdown looks quite jarring in a help message

I agree. It would be nice to be able to affect how clap prints help messages in CLI, and it might be not difficult to make markdown nicer in that mode. AFAIK clap doesn't provide an API for it.

So, maybe the whole PR is pointless and we should not try combining help message with README docs? Or avoid Markdown in docstrings? But it is a standard for docstrings to be Markdown, so it seems more reasonable to support it from the clap side.

@6r1d
Copy link
Contributor

6r1d commented Jun 6, 2023

To summarize our recent conversation, I like the format for the generated docs. Thank you for the detailed demo with Asciinema.

@0x009922
Copy link
Contributor Author

0x009922 commented Jun 8, 2023

See: #3509 (comment)

@0x009922 0x009922 closed this Jun 8, 2023
@nxsaken nxsaken deleted the 3509-kagami-clap-markdown branch May 28, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation changes Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants