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

Enhanced docs gen to more closely match existing docs (legacy) #692

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

prasek
Copy link
Contributor

@prasek prasek commented Oct 15, 2024

Note

Not for merging, only for reference for generating legacy docs during the transition.

What was changed

  • Use enrichments from Improved docs gen #691
  • Emit cmd-options.mdx with collated on option description
  • Surface different option descriptions and command usages to make it easier to combine into existing cmd-options.mdx

See this branch for the combined approach with follow-on PRs merged in.

Note: longer term would recommend moving away from cmd-options.mdx and to an inline options model similar to how tcld docs works, as some option names like name, build-id, and yes have lots of overlapping definitions and some are specific to command usage.

Why?

Needed updated operator.mdx and cmd-options.mdx output for Nexus docs and wanted to use docs gen.

Checklist

  1. How was this tested:
  1. Any docs updates needed?
  • follow on PR will generate the Nexus docs

@prasek prasek changed the title enhanced docs gen to more closely match existing docs Enhanced docs gen to more closely match existing docs Oct 15, 2024
@prasek prasek requested a review from yuandrew October 15, 2024 19:54
for _, options := range w.optionsStack {
allOptions = append(allOptions, options...)
}
if len(c.Children) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great improvement here! The old code fails to handle a few edge cases with more complex subcommands.

How are Parent and Children used? I only see Children being used to check if len(c.Children) == 0 {, and I don't see the Parent information used anywhere. If that's the case, I feel like a simpler approach would be to keep a IsLeafCommand from enrich.go and use that for this condition here

Copy link
Contributor Author

@prasek prasek Oct 30, 2024

Choose a reason for hiding this comment

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

@yuandrew Thanks! Parent is used in EnrichCommands() for various things including setting Children on the Parent. Are you thinking an IsLeafCommand() helper or something on the Command struct?

Children gives docs.go more flexibility in how the docs are rendered, for example the more hierarchical docs gen style used in this tcld PR which we probably want to move to for temporal at some point.

josh-berry pushed a commit that referenced this pull request Oct 16, 2024
## 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]>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this separate concept needs to exist. Just put inside parse or put inside docs.

Copy link
Contributor Author

@prasek prasek Oct 30, 2024

Choose a reason for hiding this comment

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

sounds good, will combine.

@prasek prasek marked this pull request as ready for review October 30, 2024 00:17
@prasek prasek marked this pull request as draft October 30, 2024 01:41
@prasek prasek mentioned this pull request Oct 30, 2024
@prasek prasek changed the title Enhanced docs gen to more closely match existing docs Enhanced docs gen to more closely match existing docs (legacy) Oct 30, 2024
@prasek
Copy link
Contributor Author

prasek commented Oct 30, 2024

Incorporated feedback into #691. Think we can close this PR as legacy docs / will not merge - since we're moving away from cmd-options.mdx to inline option rendering under each command.

This PR may be useful for generating legacy docs during the transition.

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