-
Notifications
You must be signed in to change notification settings - Fork 388
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
Fix settings file array parsing when no commas are present #1161
Fix settings file array parsing when no commas are present #1161
Conversation
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.
Looks good to me with only a minor/optional style improvement suggestion.
@@ -522,6 +522,9 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst) | |||
case "false": | |||
return false; | |||
|
|||
case "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.
Noticed this was missing and quickly added it. Makes sense from a layering perspective -- null should be parse-able but might not be allowed as a configurable value.
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.
What about just simplifying the whole code to bool.tryparse ?
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.
If one of the possibilities is null
, then it would probably look like:
string varName = varExprAst.VariablePath.UserPath.ToLower();
if (bool.TryParse(varName, out bool val))
{
return val;
}
if (varName == "null")
{
return null;
}
throw CreateInvalidDataExceptionFromAst(varExprAst);
I'm not sure that's simpler though -- the switch/case has a nice flow to me, and ideally the C# compiler knows better than I do about how to make it work quickly. (Although it looks like that's not the case, since I would implement a trie for optimum speed).
…l#1161) * Fix settings file array parsing when no commas are present * Add extra test * Improve comment * Add test file * Change test to accept null * Fix new-item call in PS 4
PR Summary
Fixes #1160.
The settings parsing logic didn't automatically capture arrays where elements are separated by newline.
I've fix that in the parser and added some tests to make sure it stays fixed.
PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.