Use YAML for CLI command generation by yuandrew · Pull Request #666 · temporalio/cli

yuandrew · 2024-09-13T16:52:58Z

What was changed

Moved from Markdown to YAML for CLI command generation.

This switch also fixes a bug where option set aliases weren't being persisted to commands that use them (i.e. NewTemporalScheduleCreateCommand)

Why?

More standardized format, easier to parse and add to

Checklist

Closes Switch to a common format for CLI command generation #620
How was this tested:

Passes all CI tests

Any docs updates needed?

…sn't

yuandrew · 2024-09-18T16:19:38Z

the new format does not allow enum options for a string[] type, so all of the options mentioned in #670 will be temporarily changed to honor their current type (i.e. string[] will accept any string value), with the description containing the enum options.

yuandrew · 2024-09-18T16:46:19Z

+      - name: raw
+        type: bool
+        description: Print properties without changing their format.
+        default: true


Description for this option used to be

* `--raw` (bool) - Print properties without changing their format. Defaults to true.

Which states defaults to true, but the code didn't seem to support that. Should this be removed from the description? A bool that defaults to true feels weird/doesn't make sense.

josh-berry · 2024-09-19T15:13:33Z

-	s.Command.Flags().StringVar(&s.Description, "description", "", "Endpoint description in markdown format (encoded using the configured codec server).")
-	s.Command.Flags().StringVar(&s.DescriptionFile, "description-file", "", "Endpoint description file in markdown format (encoded using the configured codec server).")
+	s.Command.Flags().StringVar(&s.Description, "description", "", "Endpoint description in markdown format (encoded using the configured codec server).\n")
+	s.Command.Flags().StringVar(&s.DescriptionFile, "description-file", "", "Endpoint description file in markdown format (encoded using the configured codec server).\n")


Same question as above—with options, I would expect this to be more of a problem, since it might mess up the formatting/spacing of options if some have a trailing \n and some don't.

good point, I've removed all \n from the options, generated output should match previous markdown formatting

josh-berry

I'll be honest that I didn't read the parser/generator changes super closely; I mostly looked at the generated output, which seems good to me. There's only one small comment about the log-format flag that's not a blocker for merging IMO. Otherwise LGTM!

josh-berry · 2024-09-20T17:40:41Z

-	s.Command.PersistentFlags().StringVar(&s.LogFormat, "log-format", "", "Log format. Options are: text, json. Defaults to: text.")
+	s.Command.PersistentFlags().StringVar(&s.LogFormat, "log-format", "text", "Log format. Options are: text, json.")


There was some reason we didn't want the default to be programmatically-encoded that I can't remember. Can you double-check this and make sure it's not changing behavior somehow?

josh-berry · 2024-09-20T17:47:30Z

Actually, one other thought: let's wait for @cretz to take a look before merging in case he spots something I didn't.

cretz

This looks great and basically exactly how I would have done it. Only comment worth noting is the request to change the package/dir name, everything else is non-blocking. Don't forget to update CONTRIBUTING.md.

cretz · 2024-09-23T12:55:25Z

+          Log level.
+          Default is "info" for most commands and "warn" for `server start-dev`.


Are we expected to retain the one-sentence-per-line approach? If so, how come done in options description but not command description? (do not need to fix, more for general discussion)

No, I want to get rid of this eventually and use a proper Markdown renderer for both the description and flags. The reason it was needed for flags is because newlines show up in the generated output, so having newlines inserted in (to-the-user) random places really doesn't look good. I think Andrew has since fixed this? In which case I have no opinion.

I think we should have a general/consistent rule for Markdown description. Right now it seems a bit inconsistent.

Agree, eventually; we can make this consistent once we have a proper Markdown formatter IMO.

cretz · 2024-09-23T12:56:05Z

+        type: string
+        description: |
+          Log format.
+          Options are: text, json.


There are more options than this, but I know this is a docs problem. No need to fix, can ignore. Henceforth in this PR I won't be commenting on things that are already a problem in main.

cretz · 2024-09-23T15:33:26Z

@@ -0,0 +1,192 @@
+// Package commandsgen is built to read the markdown format described in
+// temporalcli/commands.md and generate code from it.


This statement is not accurate anymore

cretz · 2024-09-23T15:37:28Z

 gen: temporalcli/commands.gen.go

-temporalcli/commands.gen.go: temporalcli/commandsmd/commands.md
+temporalcli/commands.gen.go: temporalcli/commandsgen/commands.md


This still references commands.md. In a perfect world this would have failed CI with something like:

No rule to make target 'temporalcli/commandsgen/commands.md', needed by 'temporalcli/commands.gen.go'

But unfortunately this Makefile is an untested/undocumented thing just sitting in the repo

oops, thanks for catching this!

yuandrew added 2 commits September 11, 2024 17:41

ported commands.md to commands.yml

ca361f3

current YAML draft

bb7c12d

yuandrew commented Sep 13, 2024

View reviewed changes

Comment thread temporalcli/commandsmd/commands.yml

yuandrew added 4 commits September 17, 2024 11:23

go run ./temporalcli/internal/cmd/gen-commands works but make all doe…

d04fb17

…sn't

make all works

1651a7b

Merge branch 'main' into command-yaml-generation

ea427a3

tests pass

07ec2cb

yuandrew commented Sep 18, 2024

View reviewed changes

yuandrew added 3 commits September 18, 2024 11:08

cleanup code, add instructions to top of YML

96b6240

don't support bool default

2cedef7

clean up from comparing commands.gen.go

b119093

yuandrew marked this pull request as ready for review September 18, 2024 18:35

yuandrew added 2 commits September 18, 2024 13:20

missed a small change

40be057

regen commands.gen.go

3b425e5

yuandrew requested review from cretz and josh-berry September 18, 2024 21:47

josh-berry reviewed Sep 19, 2024

View reviewed changes

remove all newlines from option descriptions

ee16399

yuandrew force-pushed the command-yaml-generation branch from 15a7549 to ee16399 Compare September 19, 2024 20:31

fix some more description spacing

4cf572b

josh-berry approved these changes Sep 20, 2024

View reviewed changes

cretz reviewed Sep 23, 2024

View reviewed changes

yuandrew added 2 commits September 23, 2024 08:20

PR comments

fb39d60

minor doc fix

c53171b

cretz reviewed Sep 23, 2024

View reviewed changes

missed a commands.md reference

df2e643

removing commands.md again, must have resurfaced from a rebase

f9f00c5

cretz approved these changes Sep 23, 2024

View reviewed changes

yuandrew merged commit e66af94 into temporalio:main Sep 23, 2024

BrewTestBot mentioned this pull request Oct 21, 2024

temporal 1.1.1 Homebrew/homebrew-core#195102

Merged

chaptersix mentioned this pull request Apr 29, 2026

[Feature Request] Support Markdown in CLI help output in terminal #687

Closed

		s.Command.PersistentFlags().StringVar(&s.LogFormat, "log-format", "", "Log format. Options are: text, json. Defaults to: text.")
		s.Command.PersistentFlags().StringVar(&s.LogFormat, "log-format", "text", "Log format. Options are: text, json.")

		Log level.
		Default is "info" for most commands and "warn" for `server start-dev`.

		@@ -0,0 +1,192 @@
		// Package commandsgen is built to read the markdown format described in
		// temporalcli/commands.md and generate code from it.

Conversation

yuandrew commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

Uh oh!

yuandrew commented Sep 18, 2024

Uh oh!

yuandrew Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josh-berry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josh-berry commented Sep 20, 2024

Uh oh!

cretz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yuandrew commented Sep 13, 2024 •

edited

Loading

yuandrew Sep 18, 2024 •

edited

Loading

cretz left a comment •

edited

Loading