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

Switch to a common format for CLI command generation #620

Closed
josh-berry opened this issue Jul 11, 2024 · 5 comments · Fixed by #666
Closed

Switch to a common format for CLI command generation #620

josh-berry opened this issue Jul 11, 2024 · 5 comments · Fixed by #666
Assignees
Labels
enhancement New feature or request

Comments

@josh-berry
Copy link
Collaborator

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 and Required., and between Default is foo and Default: foo.

After some internal discussion, YAML is probably the preferred approach, with Markdown for item descriptions. For example:

- 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:
      - [-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

Another option might be to embed YAML within Markdown, however this option was discarded as it's still more complex to parse:

### temporal env set: Set environment properties.

`temporal env set --env environment -k property -v value`

Property names match CLI option names, for example '--address' and '--tls-cert-path':

`temporal env set --env prod -k address -v 127.0.0.1:7233`
`temporal env set --env prod -k tls-cert-path -v /home/my-user/certs/cluster.cert`

If the environment is not specified, the `default` environment is used.

```yaml
maximum-args: 2
ignores-missing-env: true
options:
  - [-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

(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:

  • We still don't get the nice editor formatting/syntax-highlighting for Markdown within the description fields
  • It's almost too flexible in some ways; its duck-typing can be surprising, and there are subtleties to the indentation that can be hard to get right. However, most of this can be handled with strict validation at parse time (since we are parsing the doc into a more strongly-typed structure internally before emitting CLI commands).

Further comments and opinions are of course welcome. :)

@josh-berry josh-berry added the enhancement New feature or request label Jul 11, 2024
@josh-berry
Copy link
Collaborator Author

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.

@josh-berry
Copy link
Collaborator Author

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 

@cretz
Copy link
Member

cretz commented Jul 24, 2024

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

@josh-berry
Copy link
Collaborator Author

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.

@dandavison
Copy link
Contributor

yuandrew added a commit that referenced this issue Sep 23, 2024
## 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants