Skip to content

Change "message" in Get-Credential to mandatory#2315

Closed
iSazonov wants to merge 5 commits intoPowerShell:masterfrom
iSazonov:getcredential
Closed

Change "message" in Get-Credential to mandatory#2315
iSazonov wants to merge 5 commits intoPowerShell:masterfrom
iSazonov:getcredential

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Sep 20, 2016

Revert change from PR #1904

  1. Set "message" to optional is not best practice, from https://msdn.microsoft.com/en-us/library/dd878348%28v=vs.85%29.aspx :
    •Each parameter set must have at least one unique parameter. If possible, make this parameter a mandatory parameter.
    Set both "message" and "title" to optional open a way to "anonymous" request of credential. That is bad. This provokes writing bad code.
  2. Set "message" to optional also breaks backward compatibility: script running in version 6 will not work in the previous versions if "message" is empty.

Change "message" in Get-Credential to mandatory
@msftclas
Copy link
Copy Markdown

Hi @iSazonov, 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;

Change test to check mandatory message parameter
Check exception
ParameterArgumentValidationError,Microsoft.PowerShell.Commands.GetCredentialCommand
@mirichmo
Copy link
Copy Markdown
Member

@lzybkr Please take a look. I think you have been dealing with the iterations of this issue.

@lzybkr
Copy link
Copy Markdown
Contributor

lzybkr commented Sep 21, 2016

I tested these changes in an interactive session and it does not fix #2306, so it seems we assumed incorrectly that making -Message optional introduced that problem.

Did you want to investigate a fix for #2306 before we accept this PR?

@lzybkr
Copy link
Copy Markdown
Contributor

lzybkr commented Sep 21, 2016

Never mind about fixing #2306 - the fix is/will be in #2330.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Sep 22, 2016

Clear about #2306. I removed Close #2306.

Did you want to investigate a fix for #2306 before we accept this PR?

I believe that this PR #2315 must be accepted before #2330, because in #2330 already added tests (for "It "Get-Credential without parameters" { " and so on) - @daxian-dbw made unnecessary work.

I update fist comment of the PR with arguments.

@lzybkr
Copy link
Copy Markdown
Contributor

lzybkr commented Sep 26, 2016

I tried the changes made by @daxian-dbw - I think it provides exactly the experience we want without making -Message mandatory.

Having a mandatory parameter in a parameter set is a guideline, not a requirement. In this specific case, there are benefits to not making -Message mandatory - one can pass a custom -Title and use the default localized message.

And correct me if I'm wrong, but are there really compatibility issues? All previous versions of PowerShell support calling Get-Credential with no parameters, #2330 just improves the user experience.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

I revoke "compatibility" statement. In previous versions, an user only receives one more question, but a script will work.

Having a mandatory parameter in a parameter set is a guideline, not a requirement

Yes, it is not a requirement but the guide says about best practice for script writer that comes from the best user experience.

But! I was very conservative. I modeled various combinations of parameters (default and non-default, localized and non-localized) and think that as best practice the script writers will have to specify the message and title in a pair without the requirement that message is mandatory.

So you can dicard the PR. @lzybkr thanks for great discussion!

@lzybkr
Copy link
Copy Markdown
Contributor

lzybkr commented Sep 27, 2016

Thanks @iSazonov - I really appreciate your attention to details like this, hopefully we'll see more contributions from you in the future.

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.

4 participants