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): Fix PS1 shim error when in different drive in PS7 #4614

Merged
merged 4 commits into from
Jan 5, 2022

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Dec 30, 2021

Description

Fix PS1 shim's content switch, change from ^(.\\[\w]:).*$ to ^(\.\\)?\w:.*$. Also enclose shim content with @() for a clear look.

Motivation and Context

PS1 shim went wrong in PowerShell 7 if target is in a different drive other than shim dir. This is introduced by Resolve-Path -Relative cmdlet's output.

In PS5:
image

In PS7:
image

Other changes are pure refactoring (enclosed raw code in @())

How Has This Been Tested?

Run Invoke-Pester ".\test\Scoop-Core.Tests.ps1"

Before:
image

After:
image

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@niheaven
Copy link
Member Author

niheaven commented Dec 30, 2021

@rashil2000 Could you please check enclosed code? I do this carefully and think they haven't changed 😄

It's only refactoring and makes them look prettier.

lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
@rashil2000
Copy link
Member

Only one minor thing: Some places use '' to enclose strings, while some use "". I know "" is used only in places that have string literals, but I think we should try to be consistent instead. Mixing them up reduces readability.

Copy link
Member

@rashil2000 rashil2000 left a comment

Choose a reason for hiding this comment

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

Changes to bash shim due to #4616

lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
@niheaven
Copy link
Member Author

niheaven commented Jan 1, 2022

Only one minor thing: Some places use '' to enclose strings, while some use "". I know "" is used only in places that have string literals, but I think we should try to be consistent instead. Mixing them up reduces readability.

Well, the two different quotes are introduced by PowerShell formatter, I'll change them all to " for better readability.

niheaven and others added 2 commits January 1, 2022 22:36
Close #4616

Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
@niheaven
Copy link
Member Author

niheaven commented Jan 5, 2022

It should be done.

@niheaven niheaven merged commit 00f7859 into develop Jan 5, 2022
@niheaven niheaven deleted the fix-shim branch January 5, 2022 06:58
@niheaven
Copy link
Member Author

niheaven commented Jan 5, 2022

@rashil2000 Could you please add some tests for newly added shims? e.g., jar, python or other types.

@rashil2000
Copy link
Member

Sure!

But I have never written tests for PowerShell before. Could you point at some examples in the codebase I can look at?

se35710 pushed a commit to se35710/scoop that referenced this pull request Mar 8, 2022
…staller#4614)

Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
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

2 participants