-
Notifications
You must be signed in to change notification settings - Fork 42
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
Switch to a common format for CLI command generation #620
Comments
Not sure if we want to do this together or separately, but it would also be nice to use a proper Markdown formatter for producing help output, especially for the long descriptions. This would allow us to re-wrap text nicely, apply bold/underline, handle links, etc. |
A quick thought on how to allow for flag reuse: commands:
- temporal env set:
summary: Set environment properties
description: |
`temporal env set --env environment -k property -v value`
Property names match CLI option names, for example '--address' and '--tls-cert-path':
[...]
maximum-args: 2
ignores-missing-env: true
options:
- --env
- key-value-pair
option-defs:
--env:
type: string
description: The name of the environment to use.
key-value-pair:
- [-k, --key]:
type: string
description: The name of the property.
required: true
- [-v, --value]:
type: string
description: The value to set the property to.
required: true |
I think that's divorcing it a bit too much commands:
temporal env set:
summary: Set environment properties
description: |
`temporal env set --env environment -k property -v value`
Property names match CLI option names, for example '--address' and '--tls-cert-path':
[...]
maximum-args: 2
ignores-missing-env: true
options:
env:
type: string
description: The name of the environment to use.
option-refs: [key-value-pair]
option-defs:
key-value-pair:
key:
shorthand: k
type: string
description: The name of the property.
required: true
value:
shorthand: v
type: string
description: The value to set the property to.
required: true Or inline but I think I like this less: commands:
temporal env set:
summary: Set environment properties
description: |
`temporal env set --env environment -k property -v value`
Property names match CLI option names, for example '--address' and '--tls-cert-path':
[...]
maximum-args: 2
ignores-missing-env: true
options:
env:
type: string
description: The name of the environment to use.
key:
shorthand: k
type: string
description: The name of the property.
required: true
tag-ref: [key-value-pair]
value:
shorthand: v
type: string
description: The value to set the property to.
required: true
tag-ref: [key-value-pair]
temporal something else:
other-stuff: other-stuff
options:
some-option-specific-to-me:
type: string
include-tag-refs: [key-value-pair] But in general, I don't think we should move all options definitions away from their use, potentially only shared ones |
Completely agree, I did so in my example only for illustrative purposes. |
|
## 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 <!--- add/delete as needed ---> 1. Closes #620 2. How was this tested: <!--- Please describe how you tested your changes/how we can test them --> Passes all CI tests 3. Any docs updates needed? <!--- update README if applicable or point out where to update docs.temporal.io -->
We should switch to a more commonly-understood format for CLI command generation that is easily consumable by "standard" parsers (like a Markdown or YAML or JSON parser). The custom parser thing we have now is quite easy to misuse—for example, there is a semantic difference between
Required
andRequired.
, and betweenDefault is foo
andDefault: foo.
After some internal discussion, YAML is probably the preferred approach, with Markdown for item descriptions. For example:
Another option might be to embed YAML within Markdown, however this option was discarded as it's still more complex to parse:
(Note: Exact structure of the YAML data is still TBD; the above examples show one possibility but there are others, to be decided at implementation time.)
I also considered but discarded XML because it's significantly harder to work with in code, and therefore much less commonly-used nowadays. I also considered but discarded JSON because it's not flexible enough in its plain-text representation; we expect to have a lot of multi-line string literals, and JSON does not do well with those.
YAML does have some caveats, but we still think it's the best choice despite those caveats:
description
fieldsFurther comments and opinions are of course welcome. :)
The text was updated successfully, but these errors were encountered: