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

doc: book cli updater #3576

Merged
merged 14 commits into from
Jul 5, 2023
Merged

Conversation

leovct
Copy link
Contributor

@leovct leovct commented Jul 4, 2023

While working on #3309, I noticed that the book cli documentation is not up to date. We should advocate for software automation to handle the task of updating the book instead of relying on manual efforts.

I suggest a simple bash script tied to the Makefile that will run in CI to check if the book is properly updated given the latest changes. Anyone can run the script locally (just like a linter) to update the book cli doc using make update-book-cli.

The script updates every file under book/cli such as cli.md and reth commands and subcommands like config.md, db.md, etc. It uses a configuration file config.json to keep track of which commands, subcommands and subsubcommands to include in the book. It's important to note that if someone introduces a new command or subcommand, the script won't be aware of it and we still need to include it manually but it helps in updating the commands and subcommands already referenced in the book.


.PHONY: update-book-cli
update-book-cli: ## Update book cli documentation.
cargo build --bin reth --features "$(FEATURES)" --profile "$(PROFILE)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure about this command but the idea is to build the code

```bash
$ reth stage --help

Usage: reth db <COMMAND>
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 also helps to avoid manual errors :)

@leovct leovct force-pushed the leovct/doc-book-cli-updater branch from 3eedbb3 to 5708a10 Compare July 4, 2023 11:50
@@ -7,6 +7,25 @@ on:
merge_group:

jobs:
up-to-date:
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 might not be the best place to add this CI job because it can't be canceled when a new commit is pushed and since it's a quite long-running job (because of the build part), it may be useful to be able to cancel in progress jobs. Feel free to move it where you want :)

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #3576 (77d9f17) into main (a743146) will decrease coverage by 8.30%.
The diff coverage is n/a.

Impacted file tree graph

see 153 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.09% <ø> (-0.04%) ⬇️
unit-tests 54.38% <ø> (-9.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 25.03% <ø> (-1.44%) ⬇️
blockchain tree 78.22% <ø> (-3.04%) ⬇️
pipeline 80.11% <ø> (-6.80%) ⬇️
storage (db) 67.06% <ø> (-6.42%) ⬇️
trie 80.84% <ø> (-14.80%) ⬇️
txpool 41.48% <ø> (-7.99%) ⬇️
networking 66.96% <ø> (-10.94%) ⬇️
rpc 51.37% <ø> (-6.63%) ⬇️
consensus 48.92% <ø> (-13.67%) ⬇️
revm 28.98% <ø> (-5.97%) ⬇️
payload builder 6.83% <ø> (ø)
primitives 77.12% <ø> (-11.18%) ⬇️

@@ -15,7 +15,7 @@ jobs:
- uses: actions/checkout@v3

- name: Try to update the book cli documentation
run: make update-book-cli
run: BUILD_PATH=/home/runner/work/reth/target && make update-book-cli
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use ./reth/target instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying rn 👍

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

overall this is ok to me as a first step, but iirc there is a better way to do this using clap, but I forget how - it should be possible for clap to generate manpages or something similar, and we could turn this into markdown

@onbjerg onbjerg added C-docs An addition or correction to our documentation C-enhancement New feature or request labels Jul 5, 2023
book/cli/cli.md Outdated Show resolved Hide resolved
@leovct
Copy link
Contributor Author

leovct commented Jul 5, 2023

overall this is ok to me as a first step, but iirc there is a better way to do this using clap, but I forget how - it should be possible for clap to generate manpages or something similar, and we could turn this into markdown

Interesting, I'm still very new to Rust. I can take a look and implement something better! :)

@onbjerg
Copy link
Member

onbjerg commented Jul 5, 2023

@leovct It might be possible to create a new binary under bin that is exclusively used to build these pages. It would work by enumerating the commands and subcommands, by using e.g. Command::get_subcommands and getting the help output by running the commands

This is a bit more advanced, but the idea basically fits what an xtask is.

This is just an idea for an improvement btw, we can stil merge this an you are free to iterate on it :)

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

Needs an adjustment in the update.sh script to work on Linux (didn't spot it before)

book/cli/update.sh Outdated Show resolved Hide resolved
@leovct
Copy link
Contributor Author

leovct commented Jul 5, 2023

binary

I really like this idea, this is powerful. I'll try to get more familiar with xtask :)

@onbjerg onbjerg enabled auto-merge July 5, 2023 14:56
@onbjerg onbjerg added this pull request to the merge queue Jul 5, 2023
auto-merge was automatically disabled July 5, 2023 15:05

Head branch was pushed to by a user without write access

@leovct
Copy link
Contributor Author

leovct commented Jul 5, 2023

I can't figure how to solve the issue of the up-to-date job. I'll just remove it for the moment.

Merged via the queue into paradigmxyz:main with commit 9309279 Jul 5, 2023
@leovct leovct deleted the leovct/doc-book-cli-updater branch July 5, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-docs An addition or correction to our documentation C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants