-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 |
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
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.
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
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
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
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
-Filecurrently provides no way to pass $true/$false as parameter values. Current behavior is that-Fileaccumulates 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