fix `powershell -version` and help by SteveL-MSFT · Pull Request #4958 · PowerShell/PowerShell

SteveL-MSFT · 2017-09-30T00:39:41Z

PowerShell -v behavior updated to align with other tools like git, curl, and bash where args after -v are 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

removed test that is no longer valid due to change in -version behavior to return error

daxian-dbw · 2017-09-30T00:54:08Z

The same test didn't fail in AppVeyor. In AppVeyor, the failed test is Version should write an error if a value is present, see https://ci.appveyor.com/project/PowerShell/powershell/build/6.0.0-beta.7-5642#L384

On Travis, a different test failed: Version should ignore other parameters. See https://travis-ci.org/PowerShell/PowerShell/jobs/281514325#L1461

So probably we shouldn't just remove that test.

SteveL-MSFT · 2017-09-30T01:05:01Z

We still need to remove this test as the change is to return an error if there's anything after -version whereas previously we silently ignored it. I think the current behavior is correct.

SteveL-MSFT · 2017-09-30T01:26:34Z

Need to fix the other test and also make a change to the -v handling. I should have caught this in the code review. Apologies.

fixed `-v X` so that it errors out correctly with proper exit code updated test to catch this

iSazonov

Leave a comment

iSazonov · 2017-09-30T17:49:40Z

test/powershell/Host/ConsoleHost.Tests.ps1

-            $expectedError = (Get-Content $tempFile)[0]
-
-            $expectedError | Should Match $versionValue
+            $observed = & $powershell -version $versionValue 2>&1


😕 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.

The test I removed was when we supported:

poweshell -version -command 1+1

I 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.

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.

address PR feedback

iSazonov · 2017-09-30T19:09:19Z

src/Microsoft.PowerShell.ConsoleHost/resources/CommandLineParameterParserStrings.resx

  </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>


I still unlike this:

`-Version``. '-Version' - looks bad.

Unsupported value - implies that there are supported values.

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.

For now, I think I'll follow the other tools and will update the documentation as well.

Initial idea (in #4834) for PowerShell was to inform users that we change -Version semantics. So I write information error and PowerShell version too.

make -v have behavior consistent with other tools like git, curl, bash where args after -v are ignored removed duplicate but not exactly help text that wasn't being used in the resource file removed parameters from help that are not support in PSCore6

SteveL-MSFT · 2017-09-30T21:31:52Z

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

iSazonov · 2017-10-01T03:45:34Z

One concern is that we show full help in case of an error.
It's very annoying in interactive session if I typo - I'm forced to scroll back to see error message and lost my history before eyes. I'd prefer to see an error message and shot hint "Use powershell -? or -h to get more help" .

SteveL-MSFT · 2017-10-01T03:53:18Z

@iSazonov you referring to existing behavior and not something I introduced, correct?

iSazonov · 2017-10-01T04:35:24Z

Yes, I wanted to say that we need more review and cleanups in the file. I see:

#if !CORE
_ShowHelp=true
maybe formattings

SteveL-MSFT · 2017-10-01T05:00:35Z

@iSazonov can submit a new PR? the #if CORECLR I was going to take care of as part of #4953

iSazonov · 2017-10-01T09:14:22Z

I'll try to review on next week.

iSazonov · 2017-10-02T06:11:36Z

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

I see that we show the full command line help in some cases but not if parameter is wrong. Should we show the help in all cases? It seems bash do so.
By default we assume a file name passed. If the file don't found we write an error (bash do so too) - to write the help is not trivial but desired fix (bash don't show a help) - is it?
I suggest reformat the help in Bash style:

sort parameter list in alphabetical order.
replace long header on powershell[.exe] [options]

Maybe add examples in end of the help.

SteveL-MSFT · 2017-10-02T19:16:29Z

@iSazonov added comment and closed #4834

we should be consistent and, in general, use existing norms like bash
we should not output help either
agree with the proposed changes
absolutely!

[feature]

572b365

removed test that is no longer valid due to change in -version behavior to return error

SteveL-MSFT assigned iSazonov Sep 30, 2017

SteveL-MSFT requested a review from iSazonov September 30, 2017 00:39

SteveL-MSFT mentioned this pull request Sep 30, 2017

fix the rest of the files with CRLF to just LF #4956

Merged

markekraus mentioned this pull request Sep 30, 2017

Replace httpbin.org/gzip Tests with WebListener and Re-Enable Deflate Tests #4948

Merged

SteveL-MSFT requested review from anmenaga, daxian-dbw and lzybkr as code owners September 30, 2017 01:39

[feature]

7d1f12f

fixed `-v X` so that it errors out correctly with proper exit code updated test to catch this

SteveL-MSFT force-pushed the version-test branch from 3ebce30 to 7d1f12f Compare September 30, 2017 02:09

SteveL-MSFT mentioned this pull request Sep 30, 2017

enable support of folders and files with colon in name on Unix #4959

Merged

iSazonov reviewed Sep 30, 2017

View reviewed changes

SteveL-MSFT changed the title ~~removed test that is no longer valid for values after powershell -version~~ fix powershell -version test Sep 30, 2017

[feature]

863a28e

address PR feedback

iSazonov reviewed Sep 30, 2017

View reviewed changes

SteveL-MSFT changed the title ~~fix powershell -version test~~ fix powershell -version and help Sep 30, 2017

[feature]

56028b3

make -v have behavior consistent with other tools like git, curl, bash where args after -v are ignored removed duplicate but not exactly help text that wasn't being used in the resource file removed parameters from help that are not support in PSCore6

SteveL-MSFT force-pushed the version-test branch from b235f4f to 56028b3 Compare September 30, 2017 20:15

SteveL-MSFT mentioned this pull request Oct 1, 2017

correct passing of parameter so args show up as args instead of input #4963

Merged

iSazonov merged commit 2e901e5 into PowerShell:master Oct 1, 2017

SteveL-MSFT deleted the version-test branch October 1, 2017 03:53

SteveL-MSFT mentioned this pull request Oct 2, 2017

powershell -version 2 should return error #4834

Closed

SteveL-MSFT mentioned this pull request Oct 4, 2017

beta.8 changelog draft #5006

Merged

Conversation

SteveL-MSFT commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Sep 30, 2017

Uh oh!

SteveL-MSFT commented Sep 30, 2017

Uh oh!

SteveL-MSFT commented Sep 30, 2017

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 30, 2017

Uh oh!

iSazonov commented Oct 1, 2017

Uh oh!

SteveL-MSFT commented Oct 1, 2017

Uh oh!

iSazonov commented Oct 1, 2017

Uh oh!

SteveL-MSFT commented Oct 1, 2017

Uh oh!

iSazonov commented Oct 1, 2017

Uh oh!

iSazonov commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SteveL-MSFT commented Sep 30, 2017 •

edited

Loading

iSazonov Sep 30, 2017 •

edited

Loading

iSazonov Sep 30, 2017 •

edited

Loading

iSazonov commented Oct 2, 2017 •

edited

Loading