cli: normalize `_` → `-` when parsing options by addaleax · Pull Request #23020 · nodejs/node · GitHub

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 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/975/pipeline

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)

@targos Yeah, should be fixed now :)

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

Code LGTM

REPLACEME -> 23020?

Yup, done!

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 _?

@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…

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

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] = '-';
}

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

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

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).

The one you suggested.

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.

The no-non-const-references rule

Where can I find a reference to that?

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?

@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.

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

❤️

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.

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?

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.

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.

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

Landed in 0227635.

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 <targos@protonmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

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

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

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

cli: normalize _ → - when parsing options

671173e

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

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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 22, 2018

addaleax requested a review from danbev September 22, 2018 12:49

fixup! cli: normalize _ → - when parsing options

841cf93

targos approved these changes Sep 22, 2018

View reviewed changes

devsnek approved these changes Sep 22, 2018

View reviewed changes

refack approved these changes Sep 22, 2018

View reviewed changes

cjihrig approved these changes Sep 22, 2018

View reviewed changes

fixup! cli: normalize _ → - when parsing options

a389c08

refack reviewed Sep 22, 2018

View reviewed changes

jasnell approved these changes Sep 22, 2018

View reviewed changes

fixup! cli: normalize _ → - when parsing options

7ca87b0

BridgeAR approved these changes Sep 23, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 23, 2018

richardlau approved these changes Sep 23, 2018

View reviewed changes

danbev approved these changes Sep 25, 2018

View reviewed changes

trivikr approved these changes Sep 25, 2018

View reviewed changes

danbev closed this Sep 25, 2018

addaleax deleted the cli-underscore branch September 25, 2018 11:48

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

+              <!-- YAML
+              changes:
+                - version: REPLACEME
+                  pr-url: https://github.com/nodejs/node/pull/REPLACEME

                   if (equals_index != std::string::npos)
                     original_name += '=';
+                  {

cli: normalize `_` → `-` when parsing options by addaleax · Pull Request #23020 · nodejs/node · GitHub

Uh oh!

Uh oh!

cli: normalize _ → - when parsing options - #23020

cli: normalize _ → - when parsing options#23020addaleax wants to merge 4 commits intonodejs:masternodejs/node:masterfrom addaleax:cli-underscoreaddaleax/node:cli-underscoreCopy head branch name to clipboard

Conversation

addaleax commented Sep 22, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 22, 2018

Uh oh!

targos commented Sep 22, 2018

Uh oh!

addaleax commented Sep 22, 2018

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 22, 2018

Uh oh!

addaleax commented Sep 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 22, 2018

Uh oh!

addaleax commented Sep 22, 2018

Uh oh!

addaleax commented Sep 23, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Sep 23, 2018

Uh oh!

refack commented Sep 24, 2018

Uh oh!

addaleax commented Sep 24, 2018

Uh oh!

danbev commented Sep 25, 2018

Uh oh!

danbev commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

cli: normalize _ → - when parsing options#23020
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:cli-underscore