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

Make command-argument array public (in readonly mode) #1970

Closed
wants to merge 4 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 14, 2023

Problem

The array of registered command-arguments (currently ._args) is not made publicly available unlike the arrays of registered options (.options) and subcommands (.commands). That is not only an obvious inconsistency, but also hinders third-party library development.

Solution

Rename ._args to .registeredArguments (because .arguments is already used for a method) and add the field to the type declaration for Command in readonly mode.

Other names considered:

  • .commandArguments: confusing since the word "command" is not included in the names of .argument(), .arguments() and .addArgument()
  • .addedArguments: makes it seem like only arguments added vis .addArguments() are included

The use of the word "registered" is also consistent with the name of ._registerOption(), an internal routine suggested in #1923 and #1931.

ChangeLog

Added

  • .registeredArguments (array with Argument instances for all registered command-arguments)

Matches how command and option arrays are public.
Useful for third-party libraries.
@shadowspawn shadowspawn added the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 14, 2023
@shadowspawn
Copy link
Collaborator

This fixes #1823

registeredArguments is potentially better than declaredArguments which was the best I came up with in:
#1823 (comment)

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.

To avoid immediately breaking existing client code which may have directly accessed _args, I suggest this._args = this.registeredArguments in constructor (and deprecating _args to remove later).

@shadowspawn shadowspawn removed the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 18, 2023
@@ -281,6 +281,9 @@ export class Command {
processedArgs: any[];
readonly commands: readonly Command[];
readonly options: readonly Option[];
readonly registeredArguments: readonly Argument[];
/** @deprecated Use `.registeredArguments` instead. */
readonly _args: readonly Argument[]; // added here for strikethrough highlighting in editors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
readonly _args: readonly Argument[]; // added here for strikethrough highlighting in editors
readonly _args: readonly Argument[];

@@ -281,6 +281,9 @@ export class Command {
processedArgs: any[];
readonly commands: readonly Command[];
readonly options: readonly Option[];
readonly registeredArguments: readonly Argument[];
/** @deprecated Use `.registeredArguments` instead. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use same style as other deprecated messages

Suggested change
/** @deprecated Use `.registeredArguments` instead. */
/** @deprecated since v11, instead use `.registeredArguments` */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thought about specifying the version, but decided against it because the property had never been supported officially.

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.

Looks noisy because the name is so much longer! But not many authors will be typing it, and best name so far (and get away from the existing args/arguments/arguments). Should be accessible for symmetry with commands and options for people adding functionality.

Couple of minor changes for the comments adding and deprecating _args to typings. (Which surprised me at first, but happy now.)

(I'll update deprecation documentation later to more closely follow match the style I am using.)

@shadowspawn
Copy link
Collaborator

Closing in favour of #2010, where will list @aweebit as co-author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants