fix powershell -version and help
#4958
Conversation
|
The same test didn't fail in AppVeyor. In AppVeyor, the failed test is On Travis, a different test failed: So probably we shouldn't just remove that test. |
|
We still need to remove this test as the change is to return an error if there's anything after |
|
Need to fix the other test and also make a change to the |
3ebce30 to
7d1f12f
Compare
| $expectedError = (Get-Content $tempFile)[0] | ||
|
|
||
| $expectedError | Should Match $versionValue | ||
| $observed = & $powershell -version $versionValue 2>&1 |
There was a 8000 problem hiding this comment.
😕 I skipped [Feature] in previous PR. I saw "CI" on top the test file but we have "Feature" tests below.
We need an idea how exclude this in future.
(I wonder that the test was passed locally.)
In CI log I see messages:
[00:08:17] Usage of '-Version -command' is not supported. '-Version' currently only returns the current PowerShell version.
[00:08:17] +
[00:08:17] Usage of '-Version abrakadabra' is not supported. '-Version' currently only returns the current PowerShell version.
The first message looks incorrect. Maybe: "Error in '-Version {0}'. Now '-Version' shows the current PowerShell version and don't accept an argument."
Also previous test helped us to catch the problem - so I prefer to don't remove it because it may test another code path in future.
There was a problem hiding this comment.
The test I removed was when we supported:
poweshell -version -command 1+1I think this syntax doesn't make sense as what it ended up doing was outputting the version string and ignoring everything after it. It seems that this should have caught an unintentional user error which is why I removed that test since the new behavior prevents that old behavior.
I agree that the error message could be better particularly in the above scenario. I'll update it.
There was a problem hiding this comment.
I think I'll change it to
Unsupported value '{0}' passed to `-Version`. '-Version' currently only returns the current PowerShell version and must be used standalone.
powershell -versionpowershell -version test
| @@ -273,6 +273,6 @@ EXAMPLES | |||
| <value>Invalid argument '{0}', did you mean:</value> | |||
| </data> | |||
| <data name="DeprecatedVersionParameter" xml:space="preserve"> | |||
| <value>Usage of '-Version {0}' is not supported. '-Version' currently only returns the current PowerShell version.</value> | |||
| <value>Unsupported value '{0}' passed to `-Version`. '-Version' only returns the current PowerShell version and must be used standalone.</value> | |||
There was a problem hiding this comment.
I still unlike this:
- `-Version``. '-Version' - looks bad.
- Unsupported value - implies that there are supported values.
There was a problem hiding this comment.
If we follow the example of some other tools like: git, curl, bash (although each of them use --version), they basically ignore anything after --version and simply output the version string. Perhaps that's the right behavior.
There was a problem hiding this comment.
For now, I think I'll follow the other tools and will update the documentation as well.
There was a problem hiding this comment.
Initial idea (in #4834) for PowerShell was to inform users that we change -Version semantics. So I write information error and PowerShell version too.
powershell -version testpowershell -version and help
b235f4f to
56028b3
Compare
|
@iSazonov if you're fine with the changes, can you merge so we can unblock other PRs? If you have specific concerns with the expected behavior, I think we can continue that discussion in a new issue. Thanks! |
|
One concern is that we show full help in case of an error. |
|
@iSazonov you referring to existing behavior and not something I introduced, correct? |
|
Yes, I wanted to say that we need more review and cleanups in the file. I see:
|
|
I'll try to review on next week. |
|
@SteveL-MSFT Please clarify. In #4834 we decided to write an error to inform users about changing -Version behavior. In the PR we remove the error. Should we write in #4834 that we reject the request now? Update after review.
|
PowerShell -vbehavior updated to align with other tools like git, curl, and bash where args after-vare silently ignored.Built-in help updated to reflect changes we've made to the console host in PSCore6 removing unsupported parameters.
Doc update aligned with these changes (and general cleanup of parameters we don't support in PSCore6) MicrosoftDocs/PowerShell-Docs#1707