Skip to content

Conversation

@larrys
Copy link
Contributor

@larrys larrys commented May 7, 2025

This is a POC and a WIP of using clap-markdown to produce usage.md. I am submitting this draft PR as a request for feedback if this is something desired.

This does highlight some updates to the doc strings that clap uses for help that should be addressed in making it consistent as it relates to formatting, and the style of the content.

If we can get this to work, we can also look at clap_mangen to produce a man page for weaver.

Ideally this would be an xtask command, and part of the build process, so it keeps the docs up to date. I could not figure a way out how to best do that inside of xtask, given the way clap-markdown works with the Cli struct. I did consider putting this in build.rs, but clap_mangen suggested an xtask, so I figured it makes sense for both to follow the same advise.

@codecov
Copy link

codecov bot commented May 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.

Project coverage is 76.3%. Comparing base (78808d5) to head (bbe54d6).
Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
crates/weaver_cli/src/registry/live_check.rs 0.0% 9 Missing ⚠️
crates/weaver_cli/src/registry/generate.rs 0.0% 8 Missing ⚠️
crates/weaver_cli/src/lib.rs 0.0% 3 Missing ⚠️
crates/weaver_cli/src/registry/mod.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #732     +/-   ##
=======================================
- Coverage   76.6%   76.3%   -0.3%     
=======================================
  Files         65      69      +4     
  Lines       5012    5034     +22     
=======================================
+ Hits        3841    3845      +4     
- Misses      1171    1189     +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/main.rs Outdated
Ok(())
}

fn generate_markdown(output_file: &Option<PathBuf>) -> Result<(), String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we generate markdown via templates, I think this is a bit confusing.

Could we create a binary we run in the build that can output this which is NOT build in the general weaver binary we deploy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my plan, but was having issues doing it in xtask. I will spend some more time now that I have it working, about how best to extract it there. I wanted to solicit feedback and help with that, and see if this was something wanted as well, and this was the first pass.

Thanks for the comment!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not as efficient or comprehensive with cargo as I'd like to be. @lquerel is probably best suited to guide that, but yeah it would be desired and glad to know you thought about it.

Copy link
Contributor

@lquerel lquerel May 10, 2025

Choose a reason for hiding this comment

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

I like the idea of having this Markdown command in an xtask. For it to work, I think the following steps are necessary:

  • Add a local dependency to the main Weaver project source in the xtask’s dependencies (crates/xtask/Cargo.toml), which should provide access to the code related to the CLI definition.
  • Create a new command invoking clap markdown on the src/cli.rs

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 have tried that, and ran into quite a bit of issues, and tried a bunch of work arounds I saw online, but not were working.

I have a commit locally that moves all the clap code to its own crate, and then I depend on just that crate in the xtask. I am cleaning it up to push up as its own commit, so we can revert it if necessary, but as more of a discussion on the changes.

I also did look at using https://crates.io/crates/clap_mangen, but due to the sub commands and sub commands on those, I was running into issues on trying to have it generate all the man pages. Would creating man pages be something we would want to tackle as well? We can move that to a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was running into the same issues in trying to generate docs from schemars.

My recommendation if we can't get it working in cargo xtask, is to just have a non-published binary we can run cargo run -p docgen that we all agree to maintain for this purpose.

See: #736

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 have it working in this PR, but it required me to move the clap configuration code into a shared library, which I called crate_cli, and then I can point that xtask at that crate. From my googling, and asking CoPilot and other tools, they all suggest you need to change your project structure to have the crates depeding on each other and you can't depend on the parent crate, to avoid circular dependencies.

@larrys
Copy link
Contributor Author

larrys commented Jun 26, 2025

I need to redo this... when I have time. Cancelling for now.

@larrys larrys closed this Jun 26, 2025
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.

3 participants