Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing $true/$false as parameter to script using -File #4178

Merged
merged 2 commits into from Aug 1, 2017

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jul 3, 2017

Using powershell.exe to execute a PowerShell script using -File currently provides no way to pass $true/$false as parameter values. Current behavior is that -File accumulates passed parameters as strings only.

Fix is to special case this based on discussion with PS-Committee to support $true/$false as parsed values to parameters. Switch values is also supported as currently documented syntax doesn't work.

Fix #4036

Doc change is MicrosoftDocs/PowerShell-Docs#1430

@daxian-dbw
Copy link
Member

The commits has changes in src/Modules/Shared/Pester which should not be included. I think you need to run git submodule update to resolve it.

@SteveL-MSFT SteveL-MSFT force-pushed the file-bool-arg branch 2 times, most recently from a26bda3 to c62eccd Compare July 17, 2017 21:55
@SteveL-MSFT
Copy link
Member Author

@daxian-dbw fixed

@SteveL-MSFT
Copy link
Member Author

@lzybkr if you get a chance, can you review this? thanks

@@ -939,8 +951,7 @@ private bool ParseFile(string[] args, ref int i, bool noexitSeen)
}
else if (!string.IsNullOrEmpty(arg) && SpecialCharacters.IsDash(arg[0]))
{
Match m = argPattern.Match(arg);
if (m.Success)
if (arg.Contains(":"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of walking the string twice, move the call to IndexOf to before the if and test offset >= 0 instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will fix

@@ -949,7 +960,15 @@ private bool ParseFile(string[] args, ref int i, bool noexitSeen)
}
else
{
_collectedArgs.Add(new CommandParameter(arg.Substring(0, offset), arg.Substring(offset + 1)));
bool? boolValue = GetBoolValue(arg.Substring(offset + 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the common case, you are creating the same string twice with arg.Substring(offiset + 1), once here, and once below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this can be made much simpler, something like:

string argValue = arg.Substring(offset + 1);
bool? boolValue = GetBoolValue(argValue);
_collectedArgs.Add(new CommandParameter(arg.Substring(0, offset), boolValue ?? argValue);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null-coalescing operator requires same type on both sides, I can use ternary operator to simplify it instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, even ternary expects same type

Copy link
Collaborator

@iSazonov iSazonov Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use out parameter.

@@ -959,7 +978,15 @@ private bool ParseFile(string[] args, ref int i, bool noexitSeen)
}
else
{
_collectedArgs.Add(new CommandParameter(null, arg));
bool? boolValue = GetBoolValue(arg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, you can simply a bunch like above with ??.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, has to be same types so can't simplify with null-coalescing or ternary operator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still simplify though, something like:

object argValue = arg.Substring(offset + 1);
 _collectedArgs.Add(new CommandParameter(arg.Substring(0, offset), GetArgPossiblyAsBool(argValue)));

And GetArgPossiblyAsBool returns object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work, can't implicitly convert object to bool/string

@@ -201,7 +201,37 @@ Describe "ConsoleHost unit tests" -tags "Feature" {
$observed[1] | Should Be "bar"
}

It "-File should return exit code from script: <Filename>" -TestCases @(
It "-File should be able to pass bool values as strings to parameters: <BoolString>" -TestCases @(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these tests reflect the real user scenario.

Passing $BoolString is completely different than passing $true.

$BoolString is getting replaced with $true, but $true gets replaced with True.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above regarding usage from different shells.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated tests for both $true and true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test that accepts a string parameter and you pass true and false - we want to make sure that when we convert to bool, the bool gets properly converted back to the correct string.

And come to think about it, there is a problem - we'll lose the case that the user specified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a problem. I've changed it so it only works with switches and added tests for other cases

@@ -858,6 +858,19 @@ private bool ParseFile(string[] args, ref int i, bool noexitSeen)
// of the script to evaluate. If -file comes before -command, it will
// treat -command as an argument to the script...

bool? GetBoolValue(string arg)
{
if (arg.Equals("$true", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be True, not $true.

Or maybe it needs to be both $true and True - because it depends on the shell how $true is expanded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the use case:

powershell -file foo.ps1 -myswitch:$true

Not clear to me when we need True as I don't think we want to support:

powershell -file foo.ps1 -myswitch:true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I understand what you're getting it by your comment below. From a non-powershell shell like bash (which I thought would be the primary use case), I would think we want to support PowerShell syntax for passing $true and $false. From within powershell, I suppose we shouldn't expect the user to escape $true to be just a string. I can make this change.

…lse cannot be passed

as a parameter/switch value.
Fix is to special case this based on discussion with PS-Committee.
@SteveL-MSFT
Copy link
Member Author

@lzybkr feedback addressed

_collectedArgs.Add(new CommandParameter(arg.Substring(0, offset), arg.Substring(offset + 1)));
string argValue = arg.Substring(offset + 1);
bool? boolValue = GetBoolValue(argValue);
if (boolValue != null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry again -
Can we use LanguagePrimitives.TryConvertTo or LanguagePrimitives.TryConvertArg?
Can we use TryGetBoolValue pattern?

                               string argValue1 = arg.Substring(offset + 1); 
                               string argValue0 = arg.Substring(0, offset);
                               if (TryGetBoolValue(argValue1, out bool boolValue))  
                               {  
                                       _collectedArgs.Add(new CommandParameter(argValue0, boolValue));  
                              }  
                               else  
                               {  
                                       _collectedArgs.Add(new CommandParameter(argValue0, argValue1));  
                               }  

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@{BoolString = '$truE'},
@{BoolString = '$falSe'},
@{BoolString = 'trUe'},
@{BoolString = 'faLse'}
Copy link
Collaborator

@iSazonov iSazonov Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a protection comment like "Don't change case!". And below too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@SteveL-MSFT
Copy link
Member Author

@lzybkr any other concerns?

@ChristopherGLewis
Copy link

C:\Temp>type test.ps1
Param( [bool] $test )
Echo $test
C:\Temp>pwsh.exe -file test.ps1 -test $false
test.ps1: Cannot process argument transformation on parameter 'test'. Cannot convert value "System.String" to type "System.Boolean". Boolean parameters accept only Boolean values and numbers, such as $True, $False, 1 or 0.

C:\Temp>pwsh.exe -command $PSVersionTable

Name Value


PSVersion 7.0.0
PSEdition Core
GitCommitId 7.0.0
OS Microsoft Windows 10.0.18363
Platform Win32NT
PSCompatibleVersions {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1
WSManStackVersion 3.0

@iSazonov
Copy link
Collaborator

@ChristopherGLewis See #10838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executing powershell script with bool parameter doesnt work
6 participants