Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg by SteveL-MSFT · Pull Request #4573 · PowerShell/PowerShell

SteveL-MSFT · 2017-08-14T23:04:17Z

Since -File is now the positional parameter for powershell.exe, made the error message consistent with
-Command when passed an invalid file.

If ambiguous arg is passed, give a better error message:

PS C:\users\poker\repos\PowerShell> powershell -no -file foo
Invalid argument '-no', did you mean:

        -nologo
        -noexit
        -noprofile
        -noninteractive
PowerShell v6.0.0-beta.5-42-g3ed9a816b2452551aaad1ac1245a1cbb35e87612-dirty
Copyright (C) Microsoft Corporation. All rights reserved.

Enable -WindowStyle to work

Fix #4351

iSazonov · 2017-08-15T09:27:39Z

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

I believe argument is better term and we can be shorter:

The argument '{0}' is not recognized as the name of a script file.

Will change

lzybkr · 2017-08-15T21:32:41Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

Should this be validParameter.StartsWith(param, StringComparer.OrdinalIgnoreCase)?

I was thinking to cover cases like: -format

I'll add a test case for this

lzybkr · 2017-08-15T21:33:32Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

I'd indent with spaces - we don't indent with tabs anywhere else that I know of.

will change

lzybkr · 2017-08-15T21:40:12Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

Showing the banner after the error is weird and not like the other errors I reviewed.

I thought that was weird in general, but it seems consistent with other uses of WriteCommandLineError. Unless you know the reason it was like this, I'll change all uses to not show the banner.

iSazonov

Leave a comment

iSazonov · 2017-08-16T10:02:39Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

#if !CORECLR is no longer relevant. Should we remove the parameters at all?

I'm fixing that as part of enabling ApartmentState PR and not part of this PR

iSazonov · 2017-08-16T10:13:10Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs

Maybe
#if !UNIX

makes sense

iSazonov · 2017-08-16T10:19:28Z

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

"did you mean:" - Do you want add something else?

The possible matches show up after this. See example above in the PR description.

iSazonov · 2017-08-16T10:20:54Z

test/powershell/Host/ConsoleHost.Tests.ps1

We should exclude this on Unix.

iSazonov · 2017-08-16T10:23:31Z

test/powershell/Host/ConsoleHost.Tests.ps1

All tests is excluded on Unix so we could use our pattern "$PSDefaultParameterValues["it:skip"] = ..."

iSazonov · 2017-08-16T11:08:45Z

test/powershell/Host/ConsoleHost.Tests.ps1

Parentheses can be removed.

Will remove

SteveL-MSFT · 2017-08-16T12:04:17Z

Not sure why some of the new tests fail in AppVeyor but not on my desktop. Figured it out, need to redirect stderr to stdout. However, I noticed the tests fail on Mac because the exit code isn't being set, but still shows as passed overall.

SteveL-MSFT · 2017-08-16T13:58:58Z

Figured out why it's failing on non-Windows. It appears that exit codes from programs have to be in the range 0-255 and anything greater than 255 is mod 256. I verified this with a simple console c# app. In this case, the value for ExitCodeBadCommandLineParameter mod 256 == 0. Hmmm.

Although a breaking change, I think we should conform to libc standards which means this error code should be 64 "command line usage error". We should also update all the other error codes that powershell console returns.

In general, I don't think this should be a problem as typically 0 means success and non-0 means error so although a breaking change, I suspect it have limited real world impact.

cc @PowerShell/powershell-committee

lzybkr

I approve the changes, but there are 2 distinct fixes in the PR, so I think there should be 2 distinct commits.

Interactive add (git add -p) is an easy way to do that.

SteveL-MSFT · 2017-08-16T21:36:30Z

@lzybkr are you referring to -WindowStyle and everything else as two commits?

lzybkr · 2017-08-16T21:38:10Z

Yeah, -WindowStyle is buried at the bottom of your PR description, so it felt like it's own thing.

SteveL-MSFT · 2017-08-16T21:38:42Z

@lzybkr sure, I can make that two commits in the same PR.

Improve error message if ambiguous arg is passed to -File

SteveL-MSFT · 2017-08-16T22:07:52Z

Split into 3 commits (Thanks @lzybkr for the tip on git add -p!)

SteveL-MSFT · 2017-08-16T22:09:38Z

@PowerShell/powershell-committee reviewed this and is ok with changing the exit codes

TravisEz13 · 2017-08-17T16:00:03Z

@iSazonov, @lzybkr and @SteveL-MSFT thanks as always!

SteveL-MSFT assigned iSazonov Aug 14, 2017

SteveL-MSFT requested a review from lzybkr August 14, 2017 23:04

msftclas added the cla-not-required label Aug 14, 2017

iSazonov reviewed Aug 15, 2017

View reviewed changes

SteveL-MSFT force-pushed the console-no branch from e18b241 to d6d9505 Compare August 15, 2017 11:08

lzybkr reviewed Aug 15, 2017

View reviewed changes

SteveL-MSFT changed the title ~~Make error message consistent when invalid script is passed to -File~~ Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg Aug 16, 2017

SteveL-MSFT requested review from JamesWTruher, TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners August 16, 2017 09:29

SteveL-MSFT force-pushed the console-no branch 2 times, most recently from a9d0bac to cb97564 Compare August 16, 2017 10:23

iSazonov reviewed Aug 16, 2017

View reviewed changes

SteveL-MSFT force-pushed the console-no branch from eb660da to 951832a Compare August 16, 2017 11:43

SteveL-MSFT force-pushed the console-no branch 2 times, most recently from bb83bb5 to ab1862b Compare August 16, 2017 13:01

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 16, 2017

SteveL-MSFT added the Breaking-Change breaking change that may affect users label Aug 16, 2017

SteveL-MSFT force-pushed the console-no branch from be79184 to 5c6b694 Compare August 16, 2017 14:12

lzybkr approved these changes Aug 16, 2017

View reviewed changes

SteveL-MSFT added 3 commits August 16, 2017 15:02

Make invalid arg to -File consistent with -Command

d9c63b7

Improve error message if ambiguous arg is passed to -File

Update powershell console exit codes to match Unix standards

4c22b10

Enable -WindowStyle to work

26f7db7

SteveL-MSFT force-pushed the console-no branch from 5c6b694 to 26f7db7 Compare August 16, 2017 22:05

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Aug 16, 2017

TravisEz13 removed their request for review August 16, 2017 23:09

iSazonov approved these changes Aug 17, 2017

View reviewed changes

iSazonov merged commit 45d1d20 into PowerShell:master Aug 17, 2017

SteveL-MSFT deleted the console-no branch August 24, 2017 03:02

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr Aug 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Aug 16, 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!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

SteveL-MSFT commented Aug 16, 2017

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Aug 16, 2017

Uh oh!

lzybkr commented Aug 16, 2017

Uh oh!

SteveL-MSFT commented Aug 16, 2017

Uh oh!

SteveL-MSFT commented Aug 16, 2017

Uh oh!

SteveL-MSFT commented Aug 16, 2017

Uh oh!

TravisEz13 commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

SteveL-MSFT commented Aug 14, 2017 •

edited

Loading

lzybkr Aug 15, 2017 •

edited

Loading

SteveL-MSFT Aug 16, 2017 •

edited

Loading

SteveL-MSFT commented Aug 16, 2017 •

edited

Loading

TravisEz13 commented Aug 17, 2017 •

edited

Loading