Add beginnings of PSScriptAnalyzer 2.0 #1476
Conversation
| { | ||
| foreach (string rulePath in configuration.RulePaths) | ||
| { | ||
| string extension = Path.GetExtension(rulePath).ToLower(); |
There was a problem hiding this comment.
Following the learnings from #1099, we should be using ToLowerInvariant. But looking at the usage in the switch statement below, we should rather be using C# pattern matching for a case insensitive comparison inside a switch statement, see here for an example: https://stackoverflow.com/a/44447980
There was a problem hiding this comment.
Not a huge fan of the reverse switch. Will replace with if
|
|
||
| public ScriptAnalyzer Build() | ||
| { | ||
| IRuleProvider ruleProvider = _ruleProviders.Count == 1 |
There was a problem hiding this comment.
To me it feels like the special case of 1 rule should be handled inside CompositeRuleProvider from a consumer's perspective.
There was a problem hiding this comment.
Yeah, it's done here for efficiency. We can decide what's best when we're further along
|
|
||
| namespace Microsoft.PowerShell.ScriptAnalyzer.Builder | ||
| { | ||
| public class ScriptAnalyzerBuilder |
There was a problem hiding this comment.
Can we please try to keep the number of publicly exposed classes/methods generally minimal and instead use internal?
There was a problem hiding this comment.
Very much agree. My intent is to try and keep things behind interfaces where it makes sense to. In this case, I'm intending for the builder to be public for hosts, but certainly we will review the API surface
|
|
||
| public static IReadOnlyDictionary<string, IRuleConfiguration> RuleConfiguration { get; } = new Dictionary<string, IRuleConfiguration>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| { "PS/AvoidUsingEmptyCatchBlock", null }, |
There was a problem hiding this comment.
Can we eliminate the hard-coding here and use something like an attribute on the rule whether to run the rule by default or rely on something like typeof/nameof ? The PS prefix should also be defined only once.
There was a problem hiding this comment.
I think some of this will change. The key part though is that I want to improve cold start performance by eliminating reflection and should also have a nice declarative way to list rules.
|
|
||
| private static IPowerShellCommandDatabase InstantiatePowerShellCommandDatabase() | ||
| { | ||
| using (Runspace runspace = RunspaceFactory.CreateRunspace()) |
There was a problem hiding this comment.
What about specifying the runspace pool as something like number of rules plus 2 (or whatever makes sense)? Just an idea. At the moment we do hard-code the default.
There was a problem hiding this comment.
This particular concept I think I will rework and we can figure out the runspace configuration after that
|
|
||
| namespace Microsoft.PowerShell.ScriptAnalyzer.Configuration.Json | ||
| { | ||
| internal class JsonConfigurationConverter : JsonConverter<JsonScriptAnalyzerConfiguration> |
There was a problem hiding this comment.
I haven't looked into details but can we please support JSONC (json with comments)?
There was a problem hiding this comment.
I'm hoping we can -- I'll see about writing some tests down the line
|
|
||
| case VariableExpressionAst varExprAst: | ||
| // $true and $false are VariableExpressionAsts, so look for them here | ||
| switch (varExprAst.VariablePath.UserPath.ToLowerInvariant()) |
| } | ||
|
|
||
| /* | ||
| internal class DataflowRuleExecutor : IRuleExecutor |
There was a problem hiding this comment.
Yeah, this didn't work initially but I'm hoping to keep it to work on to see if we can improve it. Alternatively, it might be easier to convert this to a Channel now
| Copyright = '(c) Microsoft Corporation' | ||
|
|
||
| # Description of the functionality provided by this module | ||
| Description = 'PSScriptAnalyzer provides script analysis and checks for potential code defects in the scripts by applying a group of built-in or customized rules on the scripts being analyzed.' |
There was a problem hiding this comment.
maybe add that it is a formatter as well
|
|
||
| CompatiblePSEditions = @('Core', 'Desktop') | ||
|
|
||
| # HelpInfo URI of this module |
There was a problem hiding this comment.
Maybe we should use the opportunity to remove some of those commented out things in the psd1 but HelpInfoUri could actually be useful to set
|
Merging this to the WIP fork for PSSA2. Will continue working on that branch |
Start of the PSScriptAnalyzer 2.0 project.
I'll open up issues to describe the outstanding work items I have in my mind and label/milestone them appropriately.