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

cli: normalize _- when parsing options #23020

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

This allows for option syntax similar to V8’s one, e.g. --no_warnings has the same effect as --no-warnings.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This allows for option syntax similar to V8’s one, e.g.
`--no_warnings` has the same effect as `--no-warnings`.
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. cli Issues and PRs related to the Node.js command line interface. labels Sep 22, 2018
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 22, 2018
@targos
Copy link
Member

targos commented Sep 22, 2018

A test is not happy:

AssertionError [ERR_ASSERTION]: flag should be in set: --perf_basic_prof
    at goodFlags.forEach (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-process-env-allowed-flags.js:35:12)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-process-env-allowed-flags.js:34:13)

@addaleax
Copy link
Member Author

@targos Yeah, should be fixed now :)

CI: https://ci.nodejs.org/job/node-test-pull-request/17371/

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Code LGTM

doc/api/cli.md Outdated
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/REPLACEME
Copy link
Contributor

Choose a reason for hiding this comment

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

REPLACEME -> 23020?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done!

@refack
Copy link
Contributor

refack commented Sep 22, 2018

Do we know if this has any down side? AFAIK It's not a common practice with CLI interfaces? I'd love to have some discussion around this.

I can think of some minor concerns, like: less "grepability" (of batch script or docs), or less consistency in published usage examples.

Maybe we should go the other way, and deprecate flags with _?

@addaleax
Copy link
Member Author

@refack I think we would still document and list exclusively the - versions.

The upsides here are consistency with V8 options and simplification of our own options-related code. We’ve had options like --expose_http2 and --expose_internals for the same reasons.

I would also prefer not to deprecate V8 flags in our code…

@@ -307,6 +307,13 @@ void OptionsParser<Options>::Parse(
if (equals_index != std::string::npos)
original_name += '=';

{
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO should be:

for(auto& i: name) {
  if (i == '_') i = '-';
}

So there's no need for the scope, and no use of while when there's an iterator.
ES.71

Copy link
Member Author

Choose a reason for hiding this comment

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

We still have as a rule here to avoid using non-const references., and the rule you linked doesn’t seem to apply here (this is not a for loop to begin with). If you want, this can be

for (std::string::size_type i = 0; i < name.size(); ++i) {
  if (name[i] == '_') 
    name[i] = '-';
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like the for variant better. It's simple, and probably has comparable performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. There ES.72 - Prefer a for-statement to a while-statement when there is an obvious loop variable

Copy link
Member Author

Choose a reason for hiding this comment

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

Which for variant? The no-non-const-references rule has a good reason, which is that it should be obvious what is being modified (and in your code that is name and not just i itself).

Copy link
Contributor

Choose a reason for hiding this comment

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

The one you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

The no-non-const-references rule has a good reason

I have no doubt rules have good reasons. Personally I'm assuming anything that is not const is going to be changed, but I'd be happy to discuss this in the context of updating the c++ style guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

The no-non-const-references rule

Where can I find a reference to that?

@refack
Copy link
Contributor

refack commented Sep 22, 2018

The upsides here are consistency with V8 options and simplification of our own options-related code.

I agree with the upsides. But are there any downsides?

@addaleax
Copy link
Member Author

@refack I have switched to the second for variant.

The no-non-const-references rule

Where can I find a reference to that?

It’s not been written down so far, as far as I can tell; it’s come up as part of reviews a lot, though. I’ve opened #23028.

The upsides here are consistency with V8 options and simplification of our own options-related code.

I agree with the upsides. But are there any downsides?

Nothing that you haven’t mentioned yourself, I think.

@addaleax
Copy link
Member Author

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

❤️

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 23, 2018
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

For the record, I'm not a great fan of the _ variants, but being consistent with the V8 options will simplify things, both for users and for the internals.

@richardlau
Copy link
Member

I agree with the upsides. But are there any downsides?

Nothing that you haven’t mentioned yourself, I think.

Maybe a tiny, most probably unmeasurable difference in start up due to option parsing normalisation?

@refack
Copy link
Contributor

refack commented Sep 24, 2018

Maybe a tiny, most probably unmeasurable difference in start up due to option parsing normalisation?

I believe that is negligible (maybe the added code burden, but I think we actually end up with less code).

but being consistent with the V8 options

Just a point of thought, V8 doesn't have CLI options, it's an embedded engine. We decide what options it gets. We have a legacy code path that passes our CLI options to V8 verbatim.
We need to decide if we want to make that an established practice.

@addaleax
Copy link
Member Author

but being consistent with the V8 options

Just a point of thought, V8 doesn't have CLI options, it's an embedded engine.

The V8 API is called SetFlagsFromCommandLine, so I think the name is apt, and I’m not sure our code path qualifies as a legacy one, it’s a typical thing for V8 embedders to do to use that API for command line options.

@danbev
Copy link
Contributor

danbev commented Sep 25, 2018

Re-run of failing node-test-commit-arm.

@danbev
Copy link
Contributor

danbev commented Sep 25, 2018

Landed in 0227635.

@danbev danbev closed this Sep 25, 2018
danbev pushed a commit that referenced this pull request Sep 25, 2018
This allows for option syntax similar to V8’s one, e.g.
`--no_warnings` has the same effect as `--no-warnings`.

PR-URL: #23020
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@addaleax addaleax deleted the cli-underscore branch September 25, 2018 11:48
targos pushed a commit that referenced this pull request Sep 25, 2018
This allows for option syntax similar to V8’s one, e.g.
`--no_warnings` has the same effect as `--no-warnings`.

PR-URL: #23020
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos added a commit that referenced this pull request Oct 7, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* **url**
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* **util**
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
* **Windows**
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* **Added new collaborators**:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
targos added a commit that referenced this pull request Oct 10, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
targos added a commit that referenced this pull request Oct 10, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
targos added a commit that referenced this pull request Oct 10, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
lundibundi added a commit to lundibundi/node that referenced this pull request Oct 26, 2018
Previously only V8 options supported both dashes in them (making them
equivalent), but now Node.js also supports both styles so the note can
be removed.

Refs: nodejs#23020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.