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

Steve Lee (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

Ilya (iSazonov) · 2017-08-15T09:27:39Z

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

Jason Shirk (lzybkr) · 2017-08-15T21:32:41Z

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

I was thinking to cover cases like: -format

I'll add a test case for this

Jason Shirk (lzybkr) · 2017-08-15T21:33:32Z

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

will change

Jason Shirk (lzybkr) · 2017-08-15T21:40:12Z

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.

Ilya (iSazonov)

Leave a comment

Ilya (iSazonov) · 2017-08-16T10:02:39Z

#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

Ilya (iSazonov) · 2017-08-16T10:13:10Z

Maybe
#if !UNIX

makes sense

Ilya (iSazonov) · 2017-08-16T10:19:28Z

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

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

Ilya (iSazonov) · 2017-08-16T10:20:54Z

We should exclude this on Unix.

Ilya (iSazonov) · 2017-08-16T10:23:31Z

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

Ilya (iSazonov) · 2017-08-16T11:08:45Z

Parentheses can be removed.

Will remove

Steve Lee (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.

Steve Lee (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

Jason Shirk (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.

Steve Lee (SteveL-MSFT) · 2017-08-16T21:36:30Z

Jason Shirk (@lzybkr) are you referring to -WindowStyle and everything else as two commits?

Jason Shirk (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.

Steve Lee (SteveL-MSFT) · 2017-08-16T21:38:42Z

Jason Shirk (@lzybkr) sure, I can make that two commits in the same PR.

Improve error message if ambiguous arg is passed to -File

Steve Lee (SteveL-MSFT) · 2017-08-16T22:07:52Z

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

Steve Lee (SteveL-MSFT) · 2017-08-16T22:09:38Z

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

Travis Plunk (TravisEz13) · 2017-08-17T16:00:03Z

Ilya (@iSazonov), Jason Shirk (@lzybkr) and Steve Lee (@SteveL-MSFT) thanks as always!

* Make invalid arg to -File consistent with -Command Improve error message if ambiguous arg is passed to -File * Update powershell console exit codes to match Unix standards * Enable -WindowStyle to work

Steve Lee (SteveL-MSFT) assigned Ilya (iSazonov) Aug 14, 2017

Steve Lee (SteveL-MSFT) requested a review from Jason Shirk (lzybkr) August 14, 2017 23:04

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 14, 2017

Ilya (iSazonov) reviewed Aug 15, 2017

View reviewed changes

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

Jason Shirk (lzybkr) reviewed Aug 15, 2017

View reviewed changes

Steve Lee (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

Steve Lee (SteveL-MSFT) requested review from James Truher (JamesWTruher), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga) and Dongbo Wang (daxian-dbw) as code owners August 16, 2017 09:29

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

Ilya (iSazonov) reviewed Aug 16, 2017

View reviewed changes

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

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

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

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

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

Jason Shirk (lzybkr) approved these changes Aug 16, 2017

View reviewed changes

Steve Lee (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

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

Steve Lee (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

Travis Plunk (TravisEz13) removed their request for review August 16, 2017 23:09

Ilya (iSazonov) approved these changes Aug 17, 2017

View reviewed changes

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

Steve Lee (SteveL-MSFT) deleted the console-no branch August 24, 2017 03:02

Uh oh!

Conversation

Steve Lee (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!

Jason Shirk (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!

Steve Lee (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!

Ilya (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!

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

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 16, 2017

Uh oh!

Jason Shirk (lzybkr) left a comment

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 16, 2017

Uh oh!

Jason Shirk (lzybkr) commented Aug 16, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 16, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 16, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Steve Lee (SteveL-MSFT) commented Aug 14, 2017 •

edited

Loading

Jason Shirk (lzybkr) Aug 15, 2017 •

edited

Loading

Steve Lee (SteveL-MSFT) Aug 16, 2017 •

edited

Loading

Steve Lee (SteveL-MSFT) commented Aug 16, 2017 •

edited

Loading

Travis Plunk (TravisEz13) commented Aug 17, 2017 •

edited

Loading