Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Nov 16, 2018

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
Collaborator

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

@ghost
Copy link
Author

ghost commented Nov 16, 2018

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
Copy link
Collaborator

iSazonov commented Nov 16, 2018

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
Copy link
Author

ghost commented Nov 17, 2018

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
@iSazonov
Copy link
Collaborator

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

@ghost
Copy link
Author

ghost commented Nov 18, 2018

Darn. Will fix those failing checks later today.

@iSazonov iSazonov self-assigned this Nov 19, 2018
@iSazonov iSazonov requested a review from SteveL-MSFT November 19, 2018 12:59
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 21, 2018

@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
Collaborator

@lukexjeremy Please reopen the PR and we will merge.

@ghost
Copy link
Author

ghost commented Nov 24, 2018

👍

@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
Collaborator

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

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

@lukexjeremy Thanks for your contribution!

Come back with new contributions!

@ghost
Copy link
Author

ghost commented Nov 29, 2018

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

@iSazonov
Copy link
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