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

refactor(scoop-shim): Use getopt to parse arguments #5125

Merged
merged 3 commits into from
Sep 11, 2022

Conversation

lewis-yeung
Copy link
Contributor

Description

Use getopt to parse arguments, which avoids some problems. BTW, add some logic and tweak some error messages.

Motivation and Context

These problems will be resolved:

  1. For example, run scoop shim add myapp '', the second parameter (<command_path>) gets an empty value, so Scoop will throw an exception, which is not so friendly. :

    Cannot validate argument on parameter 'Name'. The argument is null or empty. Provide an argument that is not null or empty, and then try the command again.
    
  2. For example, run scoop shim list *, the regex pattern * is invalid, so Scoop will throw an exception, which is not so friendly. :

    Invalid pattern '*' at offset 1. Quantifier {x,y} following nothing.
    
  3. The -g/--global option cannot be parsed in scoop shim .... See [Bug]scoop shim info : ERROR: Option -global not recognized. #5094.

How Has This Been Tested?

Now with this:

  1. scoop shim add myapp '' shows an error:

    ERROR <command_path> must be specified for subcommand 'add'
    
  2. scoop shim list * shows an error:

    ERROR: Invalid pattern: *
    
  3. To create a local shim myapp with arg and --global as arguments:

    scoop shim add myapp 'D:\path\myapp.exe' '--' arg --global

    Note that we have to use a QUOTED command option terminator ('--') in PowerShell CLI.

  4. To create a global shim myapp with arg as the argument:

    scoop shim add -g myapp 'D:\path\myapp.exe' arg

No major changes to other subcommands.

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

@niheaven
Copy link
Member

I'll review it in a few days, thank you for the PR.

@niheaven niheaven self-assigned this Aug 29, 2022
@lewis-yeung
Copy link
Contributor Author

@niheaven Any suggestions? :)

- Rename '$Arguments' to '$other'
- Change some help text
- Support '*' and '-g' for list
@niheaven
Copy link
Member

niheaven commented Sep 9, 2022

Just see my commit, now LGTM.

@lewis-yeung
Copy link
Contributor Author

Shall we move on?

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.

2 participants