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

Allow dynamically updating version number and version option flags and description #1933

Closed
wants to merge 4 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 3, 2023

Problem

// demo.js
const { Command } = require('commander');

const flags = '-v, --my-version';
const description = 'custom version option description';

var program = new Command();
program
  .version('1.0.0')
  .version('2.0.0', flags, description); // expectation: the version number and option's flags and description are updated
program.outputHelp(); // reality: a new option is added

var program = new Command();
program
  .version('1.0.0')
  .version('2.0.0'); // expectation: the version number is updated
program.outputHelp(); // reality: a new option is added

var program = new Command();
program
  .version('1.0.0')
  .version(program._version, flags); // expectation: the version option's flags are updated
program.outputHelp(); // reality: a new option is added

var program = new Command();
program
  .version('1.0.0')
  .version(program._version, undefined, description); // expectation: the version option's description is updated
program.outputHelp(); // reality: a new option is added

var program = new Command();
const result = program
  .version('1.0.0')
  .version(undefined, flags, description); // expectation: the version option's flags and description are updated
program.outputHelp(); // reality: the flags are not updated
console.log(result); // the version number is returned instead

// A real-world example
var program = new Command();
program.version('1.0.0');
if ('EARLY_ADOPTER' in process.env) {
  program.version('2.0.0'); // expectation: the version number is simply updated
}
program.outputHelp(); // reality: contains version option twice if early adopter
program.parse(['--version'], { from: 'user' }); // output is always '1.0.0'
EARLY_ADOPTER= node demo.js

Solution

Rework version() to make all the above examples behave as expected.

Includes various improvements to the code for the version option, including the type fix from #1930. For reasoning behind those improvements, see the commit descriptions.

ChangeLog

Added

  • support for dynamically updating version number and version option flags and description via .version()

Fixed

  • (from #1930) .version() optional parameter type

Peer PRs

…solving similar problems

Borrowed from a3f0e28 that was supposed to land in the now-closed tj#1921.
The _version and _versionOptionName properties are initialized to
undefined in the constructor. The reasoning is
- to make them visible when the Command instance is logged into the
console before .version() is called, and not only appear after suddenly
- to not break anything if the proxy use for consistent option value
handling in the legacy setOptionValueWithSource mode suggested in tj#1919
(and previously in tj#1921) is adopted
Storing the Option instance of the version option instead of just its
.attributeName() (_versionOptionName) is meaningful for cases when other
properties of the instance need to be accessed (not currently the case,
but could be in the future).
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 3, 2023
Analogous to 3c94dfd in tj#1933 for version option.

Storing the Option instance of the help option instead of just its
flags (_helpShortFlag, _helpLongFlag) is meaningful for cases when other
properties of the instance need to be accessed (not currently the case,
but could be in the future).
@shadowspawn
Copy link
Collaborator

A quick reaction. I don't think being able to call .version() twice is important. On a related note in terms of early detection by author that it does not work that way, there is the PR open to add an error for overlapping option flags.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 4, 2023

I don't think being able to call .version() twice is important.

The last example in the demo code shows it could be important.

Being able to call .version() twice with new flags and description is not that important, but added for completeness and to provide meaning to calls that currently don't have any well-defined meaning at all.

On a related note in terms of early detection by author that it does not work that way, there is the PR open to add an error for overlapping option flags.

If you mean #1923, I updated it a few hours ago so that version option flag conflicts are handled, too. But it solves a different problem.

this.optional = flags.includes('['); // A value is optional when the option is specified.
// variadic test ignores <value,...> et al which might be used to describe custom splitting of single argument
this.variadic = /\w\.\.\.[>\]]$/.test(flags); // The option can take multiple values.
this.setFlagsAndDescription(flags, description);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was quite surprised this refactor did not break the typescript-checkJS run-script. At one time I am pretty sure we were getting errors for JavaScript properties which were not declared/set in the constructor.

TypeScript may have removed that check as too strict for real-world JavaScript.

microsoft/TypeScript#22896

@shadowspawn
Copy link
Collaborator

I do not currently want this functionality or refactor of Option, thanks. The .version() support is a simple convenience to add a version option with an event handler to display the version.

I see one commit was initialising this._version and this._versionOption in the constructor. That is consistent with our normal pattern if you want to suggest that separately.

(.version() started out looking like a getter/setter for a "version" property, including returning it in .opts(). Now I don't think it should be acting as a getter and I don't want to store _version or _versionOption, so I should declare that behaviour deprecated!)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 10, 2023

I see one commit was initialising this._version and this._versionOption in the constructor. That is consistent with our normal pattern if you want to suggest that separately.

Actually, no. I'll leave it undeclared and deprecate it as moving in the preferred direction.

[edit] Opened #1954

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.

2 participants