Fix bugs in importing external rule #90
Conversation
When importing customized rules, we take Ast type instead of name to apply rules. Modify string comparsion Add check for null diagnosticRecord returned.
| .First<ParameterMetadata>(item => item.Name.ToLower(CultureInfo.CurrentCulture).EndsWith("ast", StringComparison.CurrentCulture) || | ||
| item.Name.ToLower(CultureInfo.CurrentCulture).EndsWith("token", StringComparison.CurrentCulture)); | ||
| .First<ParameterMetadata>(item => item.Name.EndsWith("ast", StringComparison.OrdinalIgnoreCase) || | ||
| item.Name.EndsWith("token", StringComparison.CurrentCulture)); |
There was a problem hiding this comment.
Should the check be case insensitive here too?
| @@ -251,7 +251,7 @@ public List<ExternalRule> GetExternalRule(string[] moduleNames) | |||
| string desc =posh.AddScript(script).Invoke()[0].ImmediateBaseObject.ToString() | |||
| .Replace("\r\n", " ").Trim(); | |||
|
|
|||
| rules.Add(new ExternalRule(funcInfo.Name, funcInfo.Name, desc, param.Name, | |||
| rules.Add(new ExternalRule(funcInfo.Name, funcInfo.Name, desc, param.ParameterType.Name, | |||
There was a problem hiding this comment.
Have you tested for the case where the type is System.Management.Automation.Language.SomeAst vs just SomeAst? Ideally there should not be any discrepancy I think.
There was a problem hiding this comment.
I don't think parser recognized just someAst...
=Get-ScriptAnalyzerRule : Unable to find type [ScriptBlockAst].
There was a problem hiding this comment.
Okay I guess that is an error on the user's side then?
|
Can you add unit tests? |
Also added try/catch to capture errors in customized rule modules and throw non-terminating errors
|
I don't see the Pester test. *.tests.ps1 |
|
Yeah I'm adding it right now |
|
Also. please ignore Appveyor errors. I am trying to integrate with ScriptAnalyzer. Will use my local repro for testing the integration. You must not see these messages anymore |
| @@ -289,12 +289,12 @@ public IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token, E | |||
|
|
|||
| // Groups rules by AstType and Tokens. | |||
| Dictionary<string, List<ExternalRule>> astRuleGroups = rules | |||
| .Where<ExternalRule>(item => item.GetParameter().EndsWith("ast", true, CultureInfo.CurrentCulture)) | |||
| .Where<ExternalRule>(item => item.GetParameter().EndsWith("ast", StringComparison.OrdinalIgnoreCase)) | |||
There was a problem hiding this comment.
Also the key of this dictionary is still the name of the parameter instead of the type of the parameter.
Also added parameterType to external rule to separate parameter type and parameter name
|
Hi Quoc and Raghu, I've made a few changes including adding a new paramType in ExternalRule and new unit tests. Please take a look. Thanks! |
|
looks good. pls merge |
Fix bugs in importing external rule
|
Thanks! |
When importing customized rules, we take Ast type instead of name to
apply rules.
Modify string comparsion
Add check for null diagnosticRecord returned.