Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg #4573

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

@@ -252,7 +252,7 @@ EXAMPLES
    <value>Processing -File '{0}' failed because the file does not have a '.ps1' extension. Specify a valid Windows PowerShell script file name, and then try again.</value>
  </data>
  <data name="ArgumentFileDoesNotExist" xml:space="preserve">
-    <value>The argument '{0}' to the -File parameter does not exist. Provide the path to an existing '.ps1' file as an argument to the -File parameter.</value>
+    <value>The term '{0}' is not recognized as the name of a script file. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.</value>


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

+                        StringBuilder possibleParameters = new StringBuilder();
+                        foreach (string validParameter in validParameters)
+                        {
+                            if (validParameter.Contains(param))


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

+                        {
+                            if (validParameter.Contains(param))
+                            {
+                                possibleParameters.Append("\n\t-");


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

+                            WriteCommandLineError(
+                                string.Format(CultureInfo.CurrentCulture, CommandLineParameterParserStrings.InvalidArgument, args[i]),
+                                showBanner: false);
+                            WriteCommandLineError(possibleParameters.ToString(), showBanner: true);


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
+            "sta",
+            "mta",
+#endif


#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

@@ -446,7 +446,6 @@ internal enum KeyboardFlag : uint
        internal const int SW_MAX = 11;


-#if !CORECLR  // ProcessWindowStyle does Not exist on CoreCLR


Maybe
#if !UNIX

makes sense

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

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

@@ -269,4 +269,7 @@ EXAMPLES
  <data name="MissingConfigurationNameArgument" xml:space="preserve">
    <value>Cannot process the command because -Configuration requires an argument that is a remote endpoint configuration name. Specify this argument and try again.</value>
  </data>
+  <data name="InvalidArgument" xml:space="preserve">
+    <value>Invalid argument '{0}', did you mean:</value>


"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

+Describe "WindowStyle argument" -Tag Feature {
+    BeforeAll {
+        $ExitCodeBadCommandLineParameter = 0xFFFD0000
+        Add-Type -Name User32 -Namespace Test -MemberDefinition @"


We should exclude this on Unix.

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

test/powershell/Host/ConsoleHost.Tests.ps1

+"@
+    }
+
+    It "-WindowStyle <WindowStyle> should work on Windows" -Skip:(!$IsWindows) -TestCases @(


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

+Describe "WindowStyle argument" -Tag Feature {
+    BeforeAll {
+        $defaultParamValues = $PSDefaultParameterValues.Clone()
+        $PSDefaultParameterValues["it:skip"] = (!$IsWindows)


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 adityapatwardhan, anmenaga, daxian-dbw, JamesWTruher and TravisEz13 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg #4573

Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg #4573

SteveL-MSFT commented Aug 14, 2017 •

edited

Loading

iSazonov Aug 15, 2017

SteveL-MSFT Aug 15, 2017

lzybkr Aug 15, 2017

SteveL-MSFT Aug 16, 2017

SteveL-MSFT Aug 16, 2017

lzybkr Aug 15, 2017 •

edited

Loading

SteveL-MSFT Aug 16, 2017

lzybkr Aug 15, 2017

SteveL-MSFT Aug 16, 2017 •

edited

Loading

iSazonov left a comment

iSazonov Aug 16, 2017

SteveL-MSFT Aug 16, 2017

iSazonov Aug 16, 2017

iSazonov Aug 16, 2017

SteveL-MSFT Aug 16, 2017

iSazonov Aug 16, 2017

SteveL-MSFT Aug 16, 2017

iSazonov Aug 16, 2017

iSazonov Aug 16, 2017

SteveL-MSFT Aug 16, 2017

iSazonov Aug 16, 2017

SteveL-MSFT Aug 16, 2017

iSazonov Aug 16, 2017

SteveL-MSFT Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017 •

edited

Loading

SteveL-MSFT commented Aug 16, 2017

lzybkr left a comment

SteveL-MSFT commented Aug 16, 2017

lzybkr commented Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017

TravisEz13 commented Aug 17, 2017 •

edited

Loading

		@@ -446,7 +446,6 @@ internal enum KeyboardFlag : uint
		internal const int SW_MAX = 11;


		#if !CORECLR // ProcessWindowStyle does Not exist on CoreCLR

Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg #4573

Make error message consistent when invalid script is passed to -File, better error when passed ambiguous arg #4573

Conversation

SteveL-MSFT commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzybkr Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

iSazonov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT commented Aug 16, 2017 • edited Loading

SteveL-MSFT commented Aug 16, 2017

lzybkr left a comment

Choose a reason for hiding this comment

SteveL-MSFT commented Aug 16, 2017

lzybkr commented Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017

SteveL-MSFT commented Aug 16, 2017

TravisEz13 commented Aug 17, 2017 • edited Loading

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