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

feat: POC for allowing flexible command taxonomy #376

Merged
merged 30 commits into from Mar 14, 2022

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Feb 16, 2022

Allowing flexible command taxonomy if the flexibleTaxonomy property in the oclif config is set to true

Examples

~/repos/salesforcecli/cli [main] : bin/dev run container function start --help
Build and run a Salesforce Function in a container.

USAGE
  $ sf run container function start [-p <value>] [-b <value>] [--clear-cache] [--no-pull] [-e <value>] [--network <value>] [-v]
~/repos/salesforcecli/cli [main] : bin/dev function run container start --help
Build and run a Salesforce Function in a container.

USAGE
  $ sf function run container start [-p <value>] [-b <value>] [--clear-cache] [--no-pull] [-e <value>] [--network <value>] [-v]
~/repos/salesforcecli/cli [main] : bin/dev plugins inspect @oclif/plugin-help
└─ @oclif/plugin-help
   ├─ version 5.1.10
   ├─ homepage https://github.com/oclif/plugin-help
   ├─ location /Users/mdonnalley/repos/salesforcecli/cli/node_modules/@oclif/plugin-help
   ├─ commands
   │  └─ help
   └─ dependencies
      └─ @oclif/core ^1.0.10 => 1.3.4
~/repos/salesforcecli/cli [main] : bin/dev inspect plugins @oclif/plugin-help
└─ @oclif/plugin-help
   ├─ version 5.1.10
   ├─ homepage https://github.com/oclif/plugin-help
   ├─ location /Users/mdonnalley/repos/salesforcecli/cli/node_modules/@oclif/plugin-help
   ├─ commands
   │  └─ help
   └─ dependencies
      └─ @oclif/core ^1.0.10 => 1.3.4

@mdonnalley mdonnalley self-assigned this Feb 16, 2022
@mdonnalley mdonnalley marked this pull request as draft February 16, 2022 21:36
@mdonnalley mdonnalley force-pushed the mdonnalley/flexible-taxonomy-poc branch from c602ddd to 53226b1 Compare February 16, 2022 21:40
@mdonnalley mdonnalley force-pushed the mdonnalley/flexible-taxonomy-poc branch 2 times, most recently from 017930a to ed4301a Compare February 25, 2022 21:49
@mdonnalley mdonnalley marked this pull request as ready for review February 25, 2022 22:02
return getPerumtations(commandId.split(':')).flatMap(c => c.join(':'))
}

export function collectUsableParts(items: string[]): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mdonnalley comments please for future generations

@mdonnalley mdonnalley force-pushed the mdonnalley/flexible-taxonomy-poc branch 4 times, most recently from a520509 to 869b42b Compare March 2, 2022 16:11
src/config/config.ts Outdated Show resolved Hide resolved
src/config/util.ts Outdated Show resolved Hide resolved
src/config/util.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/help/util.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/config.ts Outdated Show resolved Hide resolved
src/config/util.ts Outdated Show resolved Hide resolved
src/config/util.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@RodEsp RodEsp left a comment

Choose a reason for hiding this comment

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

QA

Almost everything looks good except for the last two issue listed about topic help no longer showing up.

This was all tested using test-cli, expand this for command structure.
The `command_incomplete` hook just prints out the matches using console.log().

✔️ Run .\bin\dev.cmd hello -> Get error saying missing required arg
✔️ Run .\bin\dev.cmd hello Tester -> Get error saying missing required flag
✔️ Run .\bin\dev.cmd hello Tester -f Me -> Get successful command output

✔️ .\bin\dev.cmd hello world and .\bin\dev.cmd world hello have same behavior
✔️ .\bin\dev.cmd one nested and .\bin\dev.cmd nested one have same behavior
✔️ .\bin\dev.cmd two nested and .\bin\dev.cmd nested two have same behavior
✔️ .\bin\dev.cmd nested correctly passes the matched commands to the hook
✔️ .\bin\dev.cmd nested -n correctly matches the command with that flag
❌ Topics help is no longer displayed when just running a topic

Expected, this is what you get with flexibleTaxonomy set to false:

Actual, with flexibleTaxonomy set to true:

And it's even worse without a hook set:

--help flag no longer works on topics either with or without a hook enabled. With hook:

Without hook:

src/main.ts Show resolved Hide resolved
@mdonnalley mdonnalley requested a review from RodEsp March 10, 2022 15:09
Copy link
Contributor

@RodEsp RodEsp left a comment

Choose a reason for hiding this comment

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

QA - Round 2

This was all tested using test-cli, expand this for command structure.
The `command_incomplete` hook just prints out the matches using console.log().

Every ✔️ from previous QA is still ✔️.
✔️ --help flag works as expected on topics

The below behavior is still happening but it is FAD. The docs will now reflect that users must implement a command_incomplete hook when using flexibleTaxonomy.
✔️ Topics help is no longer displayed when just running a topic

With flexibleTaxonomy set to true, and the hook described above:

Without a hook set:

@RodEsp RodEsp dismissed peternhale’s stale review March 14, 2022 13:52

Changes addressed

@RodEsp RodEsp merged commit c47c6c6 into main Mar 14, 2022
@RodEsp RodEsp deleted the mdonnalley/flexible-taxonomy-poc branch March 14, 2022 13:52
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

3 participants