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

fix(getopt): Stop split arguments in getopt() and ensure array by explicit arguments type #5326

Conversation

Valinor
Copy link
Contributor

@Valinor Valinor commented Jan 6, 2023

getops handle space with quote

Correct #5313

Currently, getops parse with split space. With this correction, getops is more resilient and can parse in a more robust way with quote. Previously, 'parse this "sort of text"' => parse/this/sort/of/text
Now, 'parse this "sort of text"' => parse/this/sort of text
This correction can now handle parsing path with space (ex: c:\Program Files).

Closes #5313

How Has This Been Tested?

Use bug description, test on some other cases

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.

@rashil2000
Copy link
Member

I'm still facing the same error:
image

@Valinor
Copy link
Contributor Author

Valinor commented Jan 8, 2023

I'm still facing the same error: image

Exact, sorry for that, bad commit. To avoid that, I added tests. It's seem ok now.

@rashil2000
Copy link
Member

Okay, it's working now

lib/getopt.ps1 Outdated Show resolved Hide resolved
test/Scoop-GetOpts.Tests.ps1 Outdated Show resolved Hide resolved
Valinor and others added 3 commits January 9, 2023 14:52
Better and more readable suggestion from @niheaven

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Better test case

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Copy link
Contributor Author

@Valinor Valinor left a comment

Choose a reason for hiding this comment

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

Thanks for your review

@niheaven niheaven changed the title 5313 bug cant add a shim to an exe with spaces in its filepath fix(getopt): Stop split arguments in getopt() and ensure array by exlicit arguments type Jan 22, 2023
@niheaven niheaven changed the title fix(getopt): Stop split arguments in getopt() and ensure array by exlicit arguments type fix(getopt): Stop split arguments in getopt() and ensure array by explicit arguments type Jan 22, 2023
@niheaven niheaven force-pushed the 5313-bug-cant-add-a-shim-to-an-exe-with-spaces-in-its-filepath branch from e9e5240 to b63c301 Compare January 22, 2023 16:50
Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

I'm not sure why $argv is split by space, but it does not break anything IMO. So this PR is LGTM.

Shit, the space is added by myself in #5210... I'll check it again.

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