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(shim): Remove character replacement in .cmd -> .ps1 shims #4914

Merged
merged 2 commits into from
May 17, 2022

Conversation

lewis-yeung
Copy link
Contributor

@lewis-yeung lewis-yeung commented May 10, 2022

Description

See #4910.

Motivation and Context

Partly fix #4910.

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.

@lewis-yeung
Copy link
Contributor Author

@rashil2000 Could you please look at this? Any suggestions?

rashil2000
rashil2000 previously approved these changes May 16, 2022
@niheaven
Copy link
Member

niheaven commented May 17, 2022

Why do you put two things in one PR?

The .cmd->.ps1 part is LGTM, but parse_app() should be called in Find-Manifest() IMO. (I forgot why I hadn't added it) If you could separate this one, I'll make some change on it.

@lewis-yeung
Copy link
Contributor Author

lewis-yeung commented May 17, 2022

@niheaven Fine. I'll separate them so that you can do the parse_app part.

EDITED: Done. And I think the following change can be included in another PR that you will make:

@@ -903,7 +903,7 @@ function applist($apps, $global) {
 }
 
 function parse_app([string] $app) {
-    if($app -match '(?:(?<bucket>[a-zA-Z0-9-]+)\/)?(?<app>.*\.json$|[a-zA-Z0-9-_.]+)(?:@(?<version>.*))?') {
+    if($app -match '^(?:(?<bucket>[a-zA-Z0-9-]+)\/)?(?<app>.*\.json$|[a-zA-Z0-9-_.]+)(?:@(?<version>.*))?$') {
         return $matches['app'], $matches['bucket'], $matches['version']
     }
     return $app, $null, $null

Ref: #4910 (comment).

@lewis-yeung lewis-yeung changed the title Fix scoop home and .cmd -> .ps1 shims fix(shim): Remove character replacement in .cmd -> .ps1 shims May 17, 2022
@niheaven
Copy link
Member

In the CHANGELOG, you should mention this PR (4914), not the issue (4910).

https://github.com/ScoopInstaller/Scoop/issues/4914 will go here though issues is in the URL.

@niheaven niheaven merged commit bb5392b into ScoopInstaller:develop May 17, 2022
@lewis-yeung
Copy link
Contributor Author

@niheaven Got it, thanks.

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.

3 participants