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

Fix settings file array parsing when no commas are present #1161

Merged

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Mar 1, 2019

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

Engine/Settings.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@bergmeister bergmeister left a 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":
Copy link
Contributor Author

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.

Copy link
Collaborator

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 ?

Copy link
Contributor Author

@rjmholt rjmholt Mar 2, 2019

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).

Tests/Engine/Settings.tests.ps1 Show resolved Hide resolved
@JamesWTruher JamesWTruher merged commit 2104f05 into PowerShell:development Mar 7, 2019
bergmeister pushed a commit to bergmeister/PSScriptAnalyzer that referenced this pull request Mar 22, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to run rules with settings file that run fine with hashtable
3 participants