GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — A new hosting model which is easier for Editor Services and other script analyzer hosting by JamesWTruher · Pull Request #1361 · PowerShell/PSScriptAnalyzer Conversation
Contributor
PR SummaryThis implements a new approach for hosting script analyzer for C# developers. It does not require the developer to manage the powershell engine or runspaces, and handles that internally. It does not need the developer to call the analyzer via pipeline.invoke. It provides a number of APIs which will make it easier for EditorServices to use ScriptAnalyzer. PR Checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsHad to make the initialization of the analyzer optional as it was clobbering the settings that had been created in the formatter Hosting seems to work, starting to work on nuget packaging …nalyzer, CommandCache, and Helper Had to make the initialization of the analyzer optional as it was clobbering the settings that had been created in the formatter Hosting seems to work, starting to work on nuget packaging …nalyzer, CommandCache, and Helper There really isn't a good way to do this as the underlying apis aren't really thread safe, but we can do _something_ to the hosted analyzer If someone is using both the hosted analyzer and the module there may be some unexpected behaviors but doing something is probably better than doing nothing. Disposing the helper seems to cause downstream problems. Add async calls to the analyzer Add a simple tostring method to the HostedAnalyzer This will check to be sure that we can actually using the nupgk in a project update the version to 1.18.4 Add tests for the api and in the reference assembly Change the apis which allow adding settings to return the settings object so they can be chained Change the order of parameters to make them the same. remote extraneous documentation xml in Get-ScriptAnalyzerRule cmdlet Fix test to match new parameter order
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Helper.Instance = new Helper(runspace.SessionStateProxy.InvokeCommand, writer); | ||
| Helper.Instance.Initialize(); | ||
|
|
||
| var ruleOrder = new string[] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
note sure if .net optimises that anyway but we could use a constant for this
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| Range updatedRange; | ||
| bool fixesWereApplied; | ||
| text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied); |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
the above bool fixesWereApplied line can be inlined just by using out bool fixesWereApplied here. Same for the Range parameter
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// PSUseConsistentWhitespace | ||
| /// PSUseConsistentIndentation | ||
| /// PSAlignAssignmentStatement | ||
| /// PSUseCorrectCasing |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
the exact list of those rules could change, I'd take them out of the comment therefore. Same for other comments
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// <summary> | ||
| /// The encapsulated results of the analyzer | ||
| /// </summary> | ||
| public class AnalyzerResult |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
please move each class/enum into its own file if it is public
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| public IEnumerable<string> ExcludeRules => excludeRules; | ||
| public IEnumerable<string> Severities => severities; | ||
| public IEnumerable<string> CustomRulePath => customRulePath; | ||
| public List<string> IncludeRules => includeRules; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is needed?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A lot of code and quite complex but if it makes it better on the PSES side of things or performance wise, then go for it. Most comments are minor but would be nice to be be addressed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| { | ||
| // the rule does not exist, that's a problem, first create it and then add the key/value pair | ||
| Dictionary<string, object> settingDictionary; | ||
| if ( ! RuleArguments.TryGetValue(rule, out settingDictionary)) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
variable declaration can be inlined: out Dictionary<string, object> settingDictionary, also in a few lines below
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| <owners>Microsoft,PowerShellTeam</owners> | ||
| <projectUrl>https://github.com/PowerShell/PSScriptAnalyzer</projectUrl> | ||
| <iconUrl>https://raw.githubusercontent.com/powershell/psscriptanalyzer/master/logo.png</iconUrl> | ||
| <licenseUrl>https://github.com/PowerShell/PowerShell/blob/master/LICENSE.txt</licenseUrl> |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| <licenseUrl>https://github.com/PowerShell/PowerShell/blob/master/LICENSE.txt</licenseUrl> | |
| <licenseUrl>https://github.com/PowerShell/PSScriptAnalyzer/blob/master/LICENSE</licenseUrl> |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Close and re-open to re-trigger CI |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm going to close this as it's not needed at the moment. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.