Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Updated Split-Path to work with UNC Roots#2788

Merged
vors merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
dchristian3188:Split-Pathdchristian3188/PowerShell:Split-PathCopy head branch name to clipboard
Dec 4, 2016
Merged

Updated Split-Path to work with UNC Roots#2788
vors merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
dchristian3188:Split-Pathdchristian3188/PowerShell:Split-PathCopy head branch name to clipboard

Conversation

@dchristian3188

Copy link
Copy Markdown
Contributor

Currently splitting a UNC root on windows will return a empty string.

Please reference:
#2301

@msftclas

Copy link
Copy Markdown

Hi @dchristian3188, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -0,0 +1,49 @@
Describe "SplitPath Tests (Windows Only)" -tags "CI" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Windows [](start = 27, length = 8)

It seems like these should just be added to test/powershell/modules/Microsoft.PowerShell.Management/Split-Path.Tests.ps1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks!

@vors vors self-assigned this Dec 1, 2016

@vors vors left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great work! Can we make tests more generic to cover Windows and non-Windows platforms?

Split-Path -Parent "usr/local/bin" | Should Be "usr${dirSep}local"
}

Context "SplitPath Tests (Windows Only)" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why it's Windows Only? Can we cover all systems in these tests? I think the attitude of #2301 (comment) is to provide a consistent experience on all platforms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me spin up a linux machine and take a stab at it.

@vors vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 1, 2016
@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 2, 2016
@vors

vors commented Dec 3, 2016

Copy link
Copy Markdown
Collaborator

Waiting on non-windows tests.

@vors vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 3, 2016
@dchristian3188

Copy link
Copy Markdown
Contributor Author

Updated Tests. Please let me know if everything looks ok

@vors vors merged commit 9c9b56d into PowerShell:master Dec 4, 2016
@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Waiting on Author labels May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.