Skip to content

Fix error message from new symbolic link missing target#13085

Merged
iSazonov merged 12 commits into
PowerShell:masterfrom
yecril71pl:patch-3
Jul 3, 2020
Merged

Fix error message from new symbolic link missing target#13085
iSazonov merged 12 commits into
PowerShell:masterfrom
yecril71pl:patch-3

Conversation

@yecril71pl
Copy link
Copy Markdown
Contributor

PR Summary

Fixes #13073 (well, at least descreases its morbidity).

PR Context

When the target value was not given and New-Item throws an exception, use the custom message format intended to be used.

PR Checklist

Display a meaningful error message when a symbolic link target is not specified!
Fix: NewArgumentNullException accepts format parameter at position 1.
@ghost ghost assigned adityapatwardhan Jul 2, 2020
Comment thread src/System.Management.Automation/engine/SessionStateContainer.cs Outdated
Comment thread src/System.Management.Automation/engine/SessionStateContainer.cs Outdated
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 2, 2020
The error message should be the same whether the value is null or empty.
I was told we cannot test the exception message because of localisation and that we should test the missing parameter instead.  I disagree because CI tests run under C locale anyway, byt here you are.
This message is currently used when we fail to create a link, so we decided to be more specific.  I also changed the name to reflect this assumption.
The new name is NewLinkTargetNotSpecified.
Comment thread src/System.Management.Automation/engine/SessionStateContainer.cs Outdated
Co-authored-by: Ilya <darpa@yandex.ru>
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
Co-authored-by: Ilya <darpa@yandex.ru>
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
@iSazonov iSazonov self-assigned this Jul 3, 2020
@iSazonov iSazonov merged commit babf027 into PowerShell:master Jul 3, 2020
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Jul 3, 2020

@yecril71pl Thanks for discovering and fixing the issue!

@yecril71pl
Copy link
Copy Markdown
Contributor Author

@yecril71pl Thanks for discovering and fixing the issue!

My pleasure. I would like to note that:

  1. I am still unhappy that -TARGET is not formally mandatory in this case.
  2. I contend that -PARAMETER:VALUE is a better syntax in most cases because it is explicit and unambiguous.

@yecril71pl yecril71pl deleted the patch-3 branch July 3, 2020 08:56
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Jul 3, 2020

I am still unhappy that -TARGET is not formally mandatory in this case.

The cmdlet is provider based. I think it is impossible to make it mandatory for all scenarios. So the fix is good.

I contend that -PARAMETER:VALUE is a better syntax in most cases because it is explicit and unambiguous

We follow common pattern for tests. It would be huge work to change a style for all tests.

@ghost
Copy link
Copy Markdown

ghost commented Aug 17, 2020

🎉v7.1.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

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.

Cryptic error message on new symbolic link missing value

3 participants