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

Expose Command.options property #1784

Closed
wants to merge 4 commits into from
Closed

Conversation

mshima
Copy link
Contributor

@mshima mshima commented Aug 29, 2022

Pull Request

Problem

I could not find a reason for options not to be exposes as this PR proposes.
Currently there is no way to introspect a command, options property is not exposed. That can be useful to build manuals, readme, and so one. I am targeting to build a GitHub Action definition https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions.
Options handlers, events and maybe others are parsed/settled at addOption. Once added, changing it can have unexpected results.

Solution

Add a readonly field with readonly type.

ChangeLog

Add options to Command type.

@mshima mshima changed the title expose options as readonly Expose Command.options property Aug 29, 2022
@shadowspawn
Copy link
Collaborator

I am not against exposing the options, but for interest this is another way of inspecting the options which will sometimes be more useful.

There is a method on the Help object:

  /** Get an array of the visible options. Includes a placeholder for the implicit help option, if there is one. */
  visibleOptions(cmd: Command): Option[];

This can be used like:

const visibleOptions = cmd.createHelp().visibleOptions(cmd));

This is used when generating the help and is public to allow other uses too.

@@ -277,6 +277,7 @@ export class Command {
processedArgs: any[];
commands: Command[];
parent: Command | null;
readonly options: ReadonlyArray<Readonly<Option>>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readonly is a reasonable suggestion, but the options and commands properties are very similar. I would prefer to keep them symmetrical, and not make the options readonly.

@shadowspawn shadowspawn changed the base branch from master to develop August 30, 2022 08:20
@shadowspawn
Copy link
Collaborator

For interest, the PR that added .commands also suggested readonly! Although possibly at the time it was shallow. #1184

@mshima
Copy link
Contributor Author

mshima commented Aug 30, 2022

@shadowspawn thanks for the quick response.

const visibleOptions = cmd.createHelp().visibleOptions(cmd));

It's not obvious, but works for me.

How commander works is somewhat not intuitive.
This doesn't works:

const option = new Option(...);
command.addOption(option);
option.env('ENV_VAR');

So the readonly should prevent errors like:

command.options.forEach(option => (option.envVar = option.name().toUpperCase()));

Won't prevent errors like:

command.options.forEach(option => option.envVar.env(option.name().toUpperCase()));

For interest, the PR that added .commands also suggested readonly! Although possibly at the time it was shallow. #1184

Command is less restrictive that option for this kind of use cases.

@@ -277,6 +277,7 @@ export class Command {
processedArgs: any[];
commands: Command[];
parent: Command | null;
readonly options: Option[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you still want to add options, I would like it after commands (one line higher) and without the readonly.

@shadowspawn
Copy link
Collaborator

Options handlers, events and maybe others are parsed/settled at addOption. Once added, changing it can have unexpected results.

This is very true. A lot gets done when a command or an option is added.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shadowspawn
Copy link
Collaborator

shadowspawn commented Sep 10, 2022

Adding a property or method is normally considered a non-breaking change. However, this is a property that some people are likely to have added themselves. I checked for some possible breakages and the ReadonlyArray initially proposed in this PR would break this particular client code. It seems like narrowing the type could be ok, but not this particular one.

playground

The addition of the commands property to TypeScript happened to land in a major version.

I think for safety, this should land in a major version too? (Being very paranoid!)

@shadowspawn
Copy link
Collaborator

Tried A & B extension and found a change in resulting type too.

playground

@shadowspawn shadowspawn added the semver: major Releasing requires a major version bump, not backwards compatible label Sep 10, 2022
@shadowspawn
Copy link
Collaborator

shadowspawn commented Sep 15, 2022

I am starting to think this might not be worth adding for now:

  • hasn't been asked for before
  • alternative (and slightly safer) approach is use Help.visibleOptions() and can suggest that when people ask
  • some chance people will get themselves into trouble, like pointed out in original post:

Options handlers, events and maybe others are parsed/settled at addOption. Once added, changing it can have unexpected results.

Do you have a preference @abetomo ?

@shadowspawn
Copy link
Collaborator

Closing as not a clear enough win for now.

Thanks for the suggestion @mshima

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Releasing requires a major version bump, not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants