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

[WIP] Separate commands implementation from specific CLI args parser. #1605

Draft
wants to merge 18 commits into
base: trunk
Choose a base branch
from

Conversation

rinatkhaziev
Copy link
Contributor

Description

This is the first step of the first step of removal of yargs in favor of commander.

The general idea is to decouple commands from the specific arguments parser.

The implementation consists of two pieces:

  1. BaseCommand - it's the base class from which the rest commands are derived from.
  2. CommandRegistry - a simple singleton that stores all registered commands and invokes them.

Missing pieces

  • Argument validation - where do we do it? How do we handle positional vs named
  • Missing the allowed arguments definition. How does data shape look for it?
  • Proper TS definitions

Pull request checklist

New release checklist

Steps to Test

Outline the steps to test and verify the PR here.

Example:

  1. Check out PR.
  2. Run npm run build
  3. Run ./dist/lib/command.js
  4. Verify example output is printed.

@rinatkhaziev rinatkhaziev marked this pull request as draft December 1, 2023 00:19
@mjangda
Copy link
Member

mjangda commented Dec 9, 2023

This looks pretty cool. I'm not totally sold on the childCommands as a property; feels like it's a bit hidden and could get a bit unwieldy. But I'd be interested to hear from others.

When you feel like this is a good enough shape / structure, can you P2 it (maybe just a follow-up comment to @sjinks discovery post) so we can get perspectives from the team?

@rinatkhaziev
Copy link
Contributor Author

@mjangda

I'm not totally sold on the childCommands as a property; feels like it's a bit hidden and could get a bit unwieldy. But I'd be interested to hear from others.

I get your concern, the flip side is that we get a nice tree structure that's easy to recursively iterate on.

From the structure standpoint I think @sjinks's idea (at least my interpretation of it) is something like this:

import { OneChildCommand } from '../commands/one-child-command';
import { TwoChildCommand } from '../commands/two-child-command';

class ParentCommand {
 childCommands: [
    OneChildCommand,
   TwoChildCommand
]

So the unwieldiness part might be mitigated.

But that's foremost on my mind too, we'll see how it goes. Re: p2 - absolutely, once we have something more palatable to show.

rinatkhaziev and others added 3 commits December 11, 2023 14:06
…true, this property would control whether auth is needed, and if so will call the authenticate method, which is currently almost verbatim copy-pasta from bin/vip.js
Copy link

sonarqubecloud bot commented Jan 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

25.0% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

src/commands/app.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
src/lib/command.ts Fixed Show fixed Hide fixed
src/lib/base-command.ts Fixed Show fixed Hide fixed
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
22.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

This pull request has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep pull requests manageable and actionable and is not a comment on the quality of this pull request nor on the work done so far. Closed PRs are still valuable to the project and their branches are preserved.

@github-actions github-actions bot closed this Apr 22, 2024
@rinatkhaziev rinatkhaziev reopened this Apr 22, 2024
@rinatkhaziev rinatkhaziev added the [Status] Keep Prevent the "stale" workflow from auto-closing label Apr 22, 2024
Copy link
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails
npm/commander ^11.1.0 🟢 7.3
Details
CheckScoreReason
Code-Review🟢 9Found 9/10 approved changesets -- score normalized to 9
Maintained🟢 105 commit(s) and 25 issue activity found in the last 90 days -- score normalized to 10
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
License🟢 10license file detected
Packaging⚠️ -1packaging workflow not detected
Security-Policy🟢 10security policy file detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 9detected GitHub workflow tokens with excessive permissions
Binary-Artifacts🟢 10no binaries found in the repo
Signed-Releases⚠️ -1no releases found
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
Fuzzing⚠️ 0project is not fuzzed
SAST🟢 9SAST tool detected but not run on all commits
Pinned-Dependencies⚠️ 1dependency not pinned by hash detected -- score normalized to 1
Vulnerabilities🟢 100 existing vulnerabilities detected

Scanned Manifest Files

package.json
  • commander@^11.1.0
  • ts-node@^10.9.1

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
22.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Keep Prevent the "stale" workflow from auto-closing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants