-
Notifications
You must be signed in to change notification settings - Fork 8
Add spfs cli documentation #1215
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
base: main
Are you sure you want to change the base?
Add spfs cli documentation #1215
Conversation
Signed-off-by: Bryce Gattis <[email protected]>
…ge landing page Signed-off-by: Bryce Gattis <[email protected]>
Oh one additional thing I'm having to do manually right now, I had to add this section at the top of the generated markdown file so the new file appears correctly in the sidebar:
NOTE: This is done automatically now in latest commits. |
Part of me thinks that instead of integrating this into every command as an argument, we could have a single, separate command line that's used only during CI and output the help for all known commands using this syntax let markdown: String = clap_markdown::help_markdown::<Cli>(); That way, this separate command could know where to store each output, and generate the index files and frontmatter as well, automating the whole process. |
Signed-off-by: BryceGattis <[email protected]>
…ryceGattis/spk into feature/spfs_cli_documentation
Signed-off-by: BryceGattis <[email protected]>
…ng correctly Signed-off-by: Bryce Gattis <[email protected]>
… command we call Signed-off-by: Bryce Gattis <[email protected]>
Signed-off-by: Bryce Gattis <[email protected]>
Signed-off-by: Bryce Gattis <[email protected]>
Signed-off-by: Bryce Gattis <[email protected]>
Signed-off-by: Bryce Gattis <[email protected]>
From the meeting today:
|
I'm working to move the new The problem is, that if I try to add
Do you have any suggestions on how to solve this? Is this just because it's missing |
Hmm, ya you'd have to add a lib.rs file and lib target to the cargo toml for that crate so that it's both a binary and library in the eyes of cargo. Happy to provide guidance if you run into issues with that |
…used outside of its own package Signed-off-by: BryceGattis <[email protected]>
I got the Was this what you were thinking? I still need to move the command to be external to |
Signed-off-by: BryceGattis <[email protected]>
Yes, I think this is what I was expecting, though I'd suggest something like |
Signed-off-by: BryceGattis <[email protected]>
Signed-off-by: BryceGattis <[email protected]>
Signed-off-by: BryceGattis <[email protected]>
I noticed the "external subcommands" are not included in the help text when you just run
Is there a way to define those subcommands differently where they automatically appear as part of the help text? I see we're currently having to manually keep up with this here (which we are behind on it seems). The above issue (where they are defined as external subcommands) is causing the |
In theory we could generate the list dynamically with some gymnastics but it wouldn't solve the issue with the help generation. I think we'll need to generate those separately if you want to include them in the docs |
Can you elaborate a bit on what the dynamic list generation would look like? What do you think about moving towards an approach where none of our commands are "external" and they somehow can register themselves with the CLI entirely in their own crates? External subcommands don't seem to have a ton of documentation in the clap docs, so it's a bit hard for me to parse what they are doing and what the ideal use case for them is. If this isn't something you think we should do, do you have any other TODOs I should hit for this PR? |
I'm not actually clear on how that would work, to be honest which is part of why we haven't done it yet. Those binaries are separated out because they are given elevated privileges when installed, and so are intentionally pulled out and only do specific things. I suppose we could use a rust build script to look at all the crates and generate the list dynamically, but that doesn't necessarily solve the issue of how you would generate documentation for everything at once. I think, the best case for the doc page is to include a note about the external subcommands in the markdown page with links to additional markdown pages for those external commands, which we can also generate. |
|
||
impl Opt { | ||
pub async fn run(&mut self, config: &spfs::Config) -> miette::Result<i32> { | ||
match &mut self.cmd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rydrman One step we could do to make this more dynamic would be to make a trait that forces us to implement a run
method on all of our subcommands. Then we could eliminate this match statement entirely (assuming we find a way to dynamically update the Command
enum with all of our externally defined commands like clean,enter,fuse etc).
Something like
pub trait Runnable {
fn run(&mut self, config: &spfs::Config) -> miette::Result<i32>;
}
The problem I came across though was that most of the subcommands are async
but not all are. Any thoughts here?
I've got this generating docs for
spfs
but didn't dospk
yet.To do this, you can run:
manually from my build folder to generate the docs.
Closes #1207
TODO:
Looks like it's not showing the external commands right now. I'm assuming that's all of the subcommands defined in their own folders like
spfs clean
spfs enter
spfs fuse
etc?