Use YAML for CLI command generation #666

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?

temporalcli/commandsmd/commands.yml

…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

temporalcli/commandsmd/commands.yml

+      - 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.

temporalcli/commandsmd/commands.yml

temporalcli/commands.gen.go

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

temporalcli/commands.gen.go

-	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

temporalcli/commands.gen.go

temporalcli/commandsmd/parse.go

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

temporalcli/commands.gen.go

-	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.

temporalcli/commandsmd/parse.go

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

temporalcli/commandsmd/commands.yml

+          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

temporalcli/commandsmd/commands.yml

+        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.

temporalcli/commandsmd/commands.yml

temporalcli/commandsmd/parse.go

temporalcli/commandsmd/commands.yml

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

temporalcli/commandsgen/parse.go

@@ -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

Makefile

@@ -4,7 +4,7 @@ all: gen build

 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

temporalcli/commandsmd/commands.yml Outdated Show resolved Hide resolved

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
7 checks passed

BrewTestBot mentioned this pull request Oct 21, 2024

temporal 1.1.1 Homebrew/homebrew-core#195102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use YAML for CLI command generation #666

Use YAML for CLI command generation #666

yuandrew commented Sep 13, 2024 •

edited

Loading

yuandrew commented Sep 18, 2024

yuandrew Sep 18, 2024 •

edited

Loading

josh-berry Sep 19, 2024

yuandrew Sep 19, 2024

josh-berry left a comment

josh-berry Sep 20, 2024

josh-berry commented Sep 20, 2024

cretz left a comment •

edited

Loading

cretz Sep 23, 2024

josh-berry Sep 23, 2024

cretz Sep 23, 2024

josh-berry Sep 23, 2024

cretz Sep 23, 2024

cretz Sep 23, 2024

cretz Sep 23, 2024

yuandrew Sep 23, 2024

		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.

Use YAML for CLI command generation #666

Use YAML for CLI command generation #666

Conversation

yuandrew commented Sep 13, 2024 • edited Loading

What was changed

Why?

Checklist

yuandrew commented Sep 18, 2024

yuandrew Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-berry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-berry commented Sep 20, 2024

cretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuandrew commented Sep 13, 2024 •

edited

Loading

yuandrew Sep 18, 2024 •

edited

Loading

cretz left a comment •

edited

Loading