feat(schema)!: Remove variadic property from CLI options definition#20
feat(schema)!: Remove variadic property from CLI options definition#20ioncakephper merged 1 commit intomainfrom
variadic property from CLI options definition#20Conversation
…tion The `variadic` property has been removed from the definition of CLI options in `cli.schema.json`. This property previously allowed specifying if an option could accept multiple values. This removal simplifies the schema and its usage, streamlining how CLI options are defined. The `README.md` has been updated to reflect this change by removing the `variadic` entry from the 'Options' table. Furthermore, two crucial sections in the `README.md`—one concerning the mandatory top-level `cli` object and another about the structure of the `default` object—have been enhanced with `[!IMPORTANT]` admonitions to improve their visibility and emphasis. BREAKING CHANGE: This commit introduces breaking changes.
Summary of ChangesHello @ioncakephper, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the definition of CLI options by removing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the variadic property from CLI option definitions, a breaking change aimed at simplifying the schema. The modifications to cli.schema.json and README.md are correct and align with this objective. My review highlights two key areas for improvement. Firstly, this change results in a feature regression: it's no longer possible to enforce a list of choices for a multi-value option, as the argument definition (the new way to handle this) lacks a choices property. Secondly, I recommend enhancing the documentation to explicitly guide users on the new pattern for defining options that accept multiple values, which is crucial for a breaking change.
| "short": { "type": "string", "pattern": "^[a-zA-Z]$" }, | ||
| "description": { "type": "string" }, | ||
| "type": { "enum": ["string", "number", "boolean"] }, | ||
| "variadic": { "type": "boolean" }, |
There was a problem hiding this comment.
Removing variadic from options introduces a feature regression regarding choices. Previously, a variadic option could have its values constrained by a choices array. The new approach would be to use an option with a variadic argument. However, the argument definition does not have a choices property.
This means it's no longer possible to enforce that multiple values for an option come from a predefined set. Was this intentional? If not, I'd recommend adding a choices property to the argument definition to restore this functionality.
| | `required` | `boolean` | Whether the option is required. Defaults to `false`. | | ||
| | `short` | `string` | The single-letter short alias (e.g., `p`). Must be `^[a-zA-Z]$`. | | ||
| | `type` | `string` | The data type. Can be `string`, `number`, `boolean`. | | ||
| | `variadic` | `boolean` | Whether the option can accept multiple values. Defaults to `false`. | |
There was a problem hiding this comment.
With the removal of the variadic property, the method for defining an option that accepts multiple values is no longer obvious. To improve clarity for users of the schema, especially given this is a breaking change, I recommend adding a note to the documentation.
For instance, you could enhance the description of the arguments property within the option object's table to explain that a variadic argument should now be used to accept multiple values. This would make the transition smoother for users.
1 participant
The
variadicproperty has been removed from the definition of CLI options incli.schema.json. This property previously allowed specifying if an option could accept multiple values. This removal simplifies the schema and its usage, streamlining how CLI options are defined.The
README.mdhas been updated to reflect this change by removing thevariadicentry from the 'Options' table. Furthermore, two crucial sections in theREADME.md—one concerning the mandatory top-levelcliobject and another about the structure of thedefaultobject—have been enhanced with[!IMPORTANT]admonitions to improve their visibility and emphasis.BREAKING CHANGE: This commit introduces breaking changes.