Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@niheaven niheaven requested a review from rashil2000 December 30, 2021 18:02
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>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants