-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Allow passing $true/$false as parameter to script using -File #4178
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
Conversation
a79de4a
to
cfa82ae
Compare
The commits has changes in |
a26bda3
to
c62eccd
Compare
@daxian-dbw fixed |
c62eccd
to
dcf00f1
Compare
@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(":")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 ??
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change
There was a problem hiding this comment.
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 @( |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
dcf00f1
to
3e1eb30
Compare
…lse cannot be passed as a parameter/switch value. Fix is to special case this based on discussion with PS-Committee.
3e1eb30
to
34be39a
Compare
@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) |
There was a problem hiding this comment.
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));
}
There was a problem hiding this comment.
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'} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
…nged to use Tester-Doer pattern
5d1d483
to
4ae724f
Compare
@lzybkr any other concerns? |
C:\Temp>type test.ps1 C:\Temp>pwsh.exe -command $PSVersionTable Name Value PSVersion 7.0.0 |
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