Type commands property

[vonagam]

Pull Request

Problem

Since parent and commands properties do not start with underscore i assume that they are not private. And i see no potential issue with adding types for them, unless i am mistaken in my initial assumption.

Solution

Added types for them, marking them as readonly to show that a user needs to use addCommand if he wants to modify them.

[shadowspawn]

Are you adding these for completeness, or because you want to use them yourself?

[vonagam]

I want to use parent access to customise options collection like in #1155 (in example you use cmd.parent inside an action) and other stuff.

I want to use commands access for lookup by name or path like in _findCommand method (but it is a private method).

[shadowspawn]

Long comment...

There isn't a strong sense of private and public data properties from the original code, other than some use of underscore. Explicit routines were added for accessing some properties like .name(). Very few got added to the TypeScript and I am using my judgement as to what makes sense for maintenance reasons when cases come up.

I am happy adding commands. In fact I added code to the README recently which uses that property! Also, without the readonly(s). The readonly only goes so deep, and no protection in javascript anyway, so I am comfortable with user beware. We have `args there without qualification and I would rather follow the same pattern.

I would rather not add parent.

1. It is interesting you used Add way to collect inherited/global options from parents #1155 as an example as part of the point of that enhancement is to remove a possible case where parent might be used.
2. parent is an implementation detail and I think for internal use rather than public consumption.
3. In particular, I missed parent when I was moving property initialisation into the constructor and would likely make it null rather than undefined if/when I do. We did that for most of the properties which are objects.

[vonagam]

Interesting, i though that you would be ok with parent since it is a simple property and against commands since in the future it might change from an array to an object for example (and propose to make findCommand public instead).

readonly commands: ReadonlyArray<Command>; - just says that a) you can't replace commands with your own array, b) that you can't add or remove commands from array. (You still can modify commands themselves.) Because if you do allow it then there will be a problem with missing or wrong parent value.

I am ok with removing readonly restrictions, i just opted for more strict version.

Well with public parent people will be able to customise merging of options from above however they like, not just being limited to presets. Also adds ability to get a root from deep nested commands.

So, should i remove parent and readonly restrictions then?

[shadowspawn]

Interesting, i though that you would be ok with parent since it is a simple property and against commands since in the future it might change from an array to an object for example.

Well, you could talk me out of that too! 😃

I am ok with removing readonly restrictions, i just opted for more strict version.

I think that was a good thing to suggest and appreciate that you did, but would prefer without for now.

So, should i remove parent and readonly restrictions then?

Yes thanks, I'm happy to add plain commands.

@abetomo will be reviewing too before it is accepted.

[abetomo]

@vonagam Thank you for your contribution!

@shadowspawn Thanks for the review!

[shadowspawn]

Commander v5.0.0 has been released.

https://github.com/tj/commander.js/releases/tag/v5.0.0