-
Notifications
You must be signed in to change notification settings - Fork 38
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
Improved docs gen #691
Improved docs gen #691
Conversation
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.
Left some small nit comments, but I'm not sure the split between this PR and #691 is what we want. I think the split we want is:
- The
cmd-options.mdx
improvements and the enrichments that are required for that. - All other enrichments and improvements to the current docs generator.
That way we can merge 2. in first, and 1. can be left open as a tool for when someone cleans up the CLI documentation.
Right now it feels like a lot is getting added in, and it's not clear to me what's actually beneficial/needed.
## What was changed - Updated `commands.yml` to address feedback on temporalio/documentation#3149 (comment) - Intended for use with the following, but also is fine to merge standalone - #691 - #692 See this branch for [the combined approach](https://github.com/prasek/temporal-cli/tree/cli-docs-gen-all) with all PRs merged in. ## Why? To create CLI docs for Nexus. ## Checklist 1. How was this tested: - `go run ./temporalcli/internal/cmd/gen-docs ` - also tested with [the combined appraoch](https://github.com/prasek/temporal-cli/tree/cli-docs-gen-all) - copied generated docs (or subset) to temporalio/documentation - `yarn start` - verified via http://localhost:3000/ Ran `go test ./...` Tested locally with: ``` go run ./cmd/temporal operator nexus endpoint create --name myendpoint --target-namespace my-target-namespace --target-task-queue my-handler-task-queue --description '## Sales Services Workflow'\''s to support Customer-to-Order generation. ## other stuff ' ``` 2. Any docs updates needed? - overall docs gen needs more alignment with the existing docs, but that is out of scope for these Nexus changes - will update temporalio/documentation#3149 with cherry picked generated content from [the combined approach](https://github.com/prasek/temporal-cli/tree/cli-docs-gen-all). --------- Signed-off-by: Phil Prasek <[email protected]> Signed-off-by: Josh Berry <[email protected]>
e399ecf
to
309bf35
Compare
Makes sense. Removed the |
Addressed all feedback and think we're good to go. |
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.
LGTM (any concerns about approach of pre-populating derivable values are non-blocking). May want to have another reviewer like @josh-berry also look before merging.
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.
I just looked at the output, not the code, and left comments based on what I saw in the generated docs. Thanks again @prasek for working on this!
Also, apologies for the delay in getting to this review. It's been in my list since you first opened the PR, and I usually try to get to them faster than this but I've been a bit scatterbrained.
Thanks @josh-berry, incorporated all your suggestions on options rendering. Kept the short name for command heading since it's also used to render the right side nav which gets cluttered if using the full name, see above. |
Addressed feedback on using derived state to keep the model small. Think this is good to go. |
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.
This is a definite improvement over what we have today. Thanks @prasek for helping out here! LMK if you need me to merge on your behalf (I forget if you have permission to push the button or not, and I always prefer to let the author do the honors :D ).
@yuandrew Have all of your comments been addressed and are you comfortable with this being merged? If so can you give an approving review? (GitHub won't dismiss your "requesting changes" review unless you supersede it with an approval.) |
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.
sorry, I've been out sick the last two days, changes look good! One minor comment, but looks great otherwise! Thanks for the improvement here Phil :)
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
bf009ca
to
1cab984
Compare
addressed @yuandrew's feedback and rebased, so @josh-berry think this is good to merge. |
What was changed
Command
model for simpler docs rendering.mdx
Why?
Nexus docs need to be created and want to use docs gen.
Checklist
go run ./temporalcli/internal/cmd/gen-docs