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

Remove Persist parameter from New-PSDrive on non-Windows platform#8291

Merged
iSazonov merged 3 commits into
masterPowerShell/PowerShell:masterfrom
unknown repositoryCopy head branch name to clipboard
Nov 29, 2018
Merged

Remove Persist parameter from New-PSDrive on non-Windows platform#8291
iSazonov merged 3 commits into
masterPowerShell/PowerShell:masterfrom
unknown repositoryCopy head branch name to clipboard

Conversation

@ghost

@ghost ghost commented Nov 16, 2018

Copy link
Copy Markdown

PR Summary

Remove the Persist parameter of New-PSDrive on non-Windows platforms, since the option is only supported on Windows.

Fix #8250.

PR Checklist

@ghost ghost requested review from adityapatwardhan and daxian-dbw as code owners November 16, 2018 09:23
@iSazonov

Copy link
Copy Markdown
Collaborator

I think a right fix is to remove the parameter on Unix-s.

@ghost

ghost commented Nov 16, 2018

Copy link
Copy Markdown
Author

Interesting. I hadn't considered that.

First thoughts on the effects of removing the parameter on non-Windows platforms.

That would entirely prevent -Persist confusion while using the shell or writing a script on a non-Windows machine 👍

However, suppose someone writes a script calling New-PSDrive -Persist ... on Windows. Then they attempt to run the script on MacOS. Is it better for debugging to throw a ParameterBindingException or a PlatformNotSupportedException?

I think the PlatformNotSupportedException is self-explanatory. OTOH, maybe the ParameterBindingException would warrant an explanation in the cmdlet help. If so, do you keep the Persist parameter help to explain that on non-Windows machines, even though the parameter isn't available? Perhaps the scenario is contrived, but worth a thought at least.

In terms of implementation, I think removing the parameter would require wrapping the Persist property in a #if !UNIX directive. Therefore, every reference to that property would also need to be wrapped in a #if !UNIX directive. For many references, I would prefer not do that. Though In this case, there is only one reference to Persist and this cmdlet seems quite stable.

What do you think?

@iSazonov

iSazonov commented Nov 16, 2018

Copy link
Copy Markdown
Collaborator

We already have an experience with removing unsupported cmdlets and parameters on Unix platforms. User will get "A parameter cannot be found ..." error that looks good. See #4922 - implemented after the comment #4897 (comment)

@ghost

ghost commented Nov 17, 2018

Copy link
Copy Markdown
Author

Perfect example to follow 👌 Thank you.

@ghost ghost changed the title WIP: Throw meaningful exception if New-PSDrive -Persist used on non-Windows platform WIP: Remove Persist parameter from New-PSDrive -Persist on non-Windows platform Nov 17, 2018
@ghost ghost changed the title WIP: Remove Persist parameter from New-PSDrive -Persist on non-Windows platform WIP: Remove Persist parameter from New-PSDrive on non-Windows platform Nov 17, 2018
@ghost ghost changed the title WIP: Remove Persist parameter from New-PSDrive on non-Windows platform Remove Persist parameter from New-PSDrive on non-Windows platform Nov 18, 2018
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/PSDrive.Tests.ps1 Outdated
@iSazonov

Copy link
Copy Markdown
Collaborator

@lukexjeremy In next time please don't put your commits in master branch - create a new work branch for every new PR.

@ghost

ghost commented Nov 18, 2018

Copy link
Copy Markdown
Author

Darn. Will fix those failing checks later today.

@iSazonov iSazonov self-assigned this Nov 19, 2018
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
@iSazonov iSazonov requested a review from SteveL-MSFT November 19, 2018 12:59
@iSazonov

iSazonov commented Nov 21, 2018

Copy link
Copy Markdown
Collaborator

@lukexjeremy Please open new issue in https://github.com/PowerShell/PowerShell-Docs to document the change.

And add here new empty commit with [Feature] header to run all tests.

@iSazonov

Copy link
Copy Markdown
Collaborator

@lukexjeremy Please reopen the PR and we will merge.

@ghost

ghost commented Nov 24, 2018

Copy link
Copy Markdown
Author

👍

@ghost ghost reopened this Nov 24, 2018
`Persist` is only supported on Windows.
* Make it clear that persist is being set to false on UNIX
* Use Pester's `-Skip` parameter instead of if statement to skip test
@iSazonov

Copy link
Copy Markdown
Collaborator

@lukexjeremy You removed commits. Could you restore your branch?

@iSazonov iSazonov merged commit 0cc1f06 into PowerShell:master Nov 29, 2018
@iSazonov

Copy link
Copy Markdown
Collaborator

@lukexjeremy Thanks for your contribution!

Come back with new contributions!

@ghost

ghost commented Nov 29, 2018

Copy link
Copy Markdown
Author

Thanks for your help too. Will do and looking forward to the next contribution.

@iSazonov

Copy link
Copy Markdown
Collaborator

You could look issues with Up-for-Grabs. Also we have an issue to hide parameters and cmdlets which is not still implemented on Unix. There are more such examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New-PSDrive -Persist should report a platform-not-supported error (System.PlatformNotSupportedException) on Unix-like platforms

3 participants

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