[ts-command-line] Add defineCommandLineRemainder() feature #1869

octogonz · 2020-05-14T09:56:53Z

This PR adds a new feature to ts-command-line that enables the remaining CLI arguments to be captured, without any validation. We want this for #1232 (comment) so that rushx can pass its arguments along to another command.

While I was in there, I fixed a bunch of other minor issues with ts-command-line:

Fix a bug with environmentVariable mapping for CommandLineFlagParameter
Add the ability for an environment variable to specify multiple values for CommandLineStringListParameter, encoded as a JSON array
Use API Extractor to trim internal APIs from the .d.ts rollup
Fix some bugs that prevented a CommandLineParser from being defined without any actions
Improve the README.md and API documentation

I also added a ts-command-line-test project that makes it easier to debug the library.

…eter - Provide a way for an CommandLineStringListParameter environmentVariable to specify multiple values using a JSON array

…er()

- Fix a bug where CommandLineParser could not be declared without actions - Prohibit defineCommandLineRemainder() on the parser if it has actions

sachinjoseph · 2020-05-14T22:35:19Z

libraries/ts-command-line/src/CommandLineParameter.ts

-        // with escaping here seems unwise, since there are so many shell escaping mechanisms that could
-        // potentially confuse the experience.
-        this._values = [ environmentValue ];
+        if (environmentValue[0] === '[') {


Can environmentVariableValue be an empty string ""?

In Bash, it is easy to assign an empty string to an environment variable:

export MY_VARIABLE=

For the DOS cmd.exe shell, it is possible for the value to be an empty string, but you cannot use SET to assign it -- doing so will delete the variable. So it has to be assigned using some other tool. PowerShell can probably do it.

If you're asking about the design here, because an empty string does not start with [, it will simply become the first item in the list.

I was asking because if environmentValue can be "" then accessing environmentValue[0] can throw whenever the string is empty but not undefined. The string can become "" if it was originally whitespace entirely and then we trimmed the string.

Yes in C#, not true in JavaScript.

sachinjoseph · 2020-05-14T22:37:34Z

libraries/ts-command-line/src/CommandLineParameter.ts

-        // with escaping here seems unwise, since there are so many shell escaping mechanisms that could
-        // potentially confuse the experience.
-        this._values = [ environmentValue ];
+        if (environmentValue[0] === '[') {


Can environmentVariableValue start with whitespace characters?

Yes, but then it will not be parsed as an array. The entire string will simply become the first item in the list.

Hmm... that might be counterintuitive if a person is typing the value into a text box in a web page (e.g. AzureDev Ops). I'll trim it. 👍

I also documented the parsing rules.

iclanton · 2020-05-15T04:49:22Z

build-tests/ts-command-line-test/build.js

@@ -0,0 +1,20 @@
+const fsx = require('fs-extra');


Why not FileSystem?

This library predates node-core-library. If we ever do an operation that would genuinely benefit, I'll add it as a dependency.

iclanton · 2020-05-15T04:50:41Z

build-tests/ts-command-line-test/tsconfig.json

+  },
+  "include": [
+    "src/**/*.ts",
+    "typings/tsd.d.ts"


This project doesn't use RSC because it's intended as a code sample.

Oh right, but tsd.d.ts isn't needed; removed.

iclanton · 2020-05-15T04:59:26Z

libraries/ts-command-line/src/CommandLineParser.ts

+    if (!this.selectedAction) {
+      return Promise.resolve();
+    }
+    return this.selectedAction._execute();


Why not turn this into an async function and:

Suggested change

if (!this.selectedAction) {

return Promise.resolve();

}

return this.selectedAction._execute();

if (this.selectedAction) {

await this.selectedAction._execute();

}

I'm going to bulk-convert everything to async/await, but not in this PR. It needs to be a standalone PR.

iclanton · 2020-05-15T04:59:44Z

libraries/ts-command-line/src/test/ActionlessParser.test.ts

+
+    return commandLineParser.execute(['--flag', 'the', 'remaining', 'args']).then(() => {
+      expect(commandLineParser.selectedAction).toBeUndefined();
+    //  expect(commandLineParser.flag.value).toBe(true);


Oops, that was not meant to be disabled. I've uncommented it.

… trimming for string list JSON arrays

octogonz · 2020-05-15T15:23:20Z

This was published as ts-command-line version 4.4.0.

octogonz added 14 commits May 13, 2020 00:55

Improve docs for CommandLineStringListParameter

bc81507

Improve README.md

b6d4827

- Fix a bug with environmentVariable mapping for CommandLineFlagParam…

67ebf53

…eter - Provide a way for an CommandLineStringListParameter environmentVariable to specify multiple values using a JSON array

Add a test case for environment variable mappings

7c69af0

More README.md tuneups

40c97c5

Add a build test project for ts-command-line

bee3458

Upgrade argparse typings

43019fd

rush update --full

cf1b4bb

Add new feature CommandLineParameterProvider.defineCommandLineRemaind…

95e46d3

…er()

Trim internal APIs from .d.ts rollup

fd256db

Add debug config

a2c9e3d

- Add tests for CommandLineRemainder

d25e672

- Fix a bug where CommandLineParser could not be declared without actions - Prohibit defineCommandLineRemainder() on the parser if it has actions

Add a test to ensure actionless parsers work correctly

5b68633

rush change

94c1c73

octogonz requested review from apostolisms, D4N14L, halfnibble, iclanton, patmill and sachinjoseph as code owners May 14, 2020 09:56

octogonz mentioned this pull request May 14, 2020

[rush] rushx should support parameters for commands #1232

Closed

Add a link to ts-command-line-test

7b013d5

sachinjoseph reviewed May 14, 2020

View reviewed changes

iclanton approved these changes May 15, 2020

View reviewed changes

octogonz added 5 commits May 14, 2020 23:50

PR feedback: document how environment variable values are parsed, add…

30e9ea2

… trimming for string list JSON arrays

Add a README.md for the ts-command-line-test project

9a4eaca

PR feedback: remove "typings/tsd.d.ts"

94b7b16

PR feedback: fix commented line

f3ad884

Fix markdown table

bcc3be1

octogonz force-pushed the octogonz/tsc-improvements branch from 4d07147 to bcc3be1 Compare May 15, 2020 07:44

Fix typo

8571c7a

octogonz merged commit 818e085 into master May 15, 2020

octogonz deleted the octogonz/tsc-improvements branch May 15, 2020 07:56

Js-Brecht mentioned this pull request May 24, 2020

[rush] Improve command-line for "rush add" #1160

Closed

octogonz mentioned this pull request Jun 8, 2020

[rush] Enable Prettier integration #1573

Closed

octogonz mentioned this pull request Feb 18, 2021

[ts-command-line] CommandLineRemainder doesn't work as expected #2499

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ts-command-line] Add defineCommandLineRemainder() feature #1869

[ts-command-line] Add defineCommandLineRemainder() feature #1869

octogonz commented May 14, 2020

sachinjoseph May 14, 2020

octogonz May 15, 2020

sachinjoseph May 15, 2020

octogonz May 15, 2020

sachinjoseph May 14, 2020

octogonz May 15, 2020

octogonz May 15, 2020

iclanton May 15, 2020

octogonz May 15, 2020

iclanton May 15, 2020

octogonz May 15, 2020

octogonz May 15, 2020

iclanton May 15, 2020

octogonz May 15, 2020

iclanton May 15, 2020

octogonz May 15, 2020

octogonz commented May 15, 2020

[ts-command-line] Add defineCommandLineRemainder() feature #1869

[ts-command-line] Add defineCommandLineRemainder() feature #1869

Conversation

octogonz commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octogonz commented May 15, 2020