A new hosting model which is easier for Editor Services and other script analyzer hosting #1361
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
bergmeister
reviewed
Jan 16, 2020
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "sdk": { | |||
| "version": "2.2.402" | |||
There was a problem hiding this comment.
.net core 2.2 has reached EOL in December already and PSSA has already moved on to 3.1.101 in the root directory. I think we can simply remove the file
bergmeister
changed the title
bergmeister
reviewed
Jan 16, 2020
| Helper.Instance = new Helper(runspace.SessionStateProxy.InvokeCommand, writer); | ||
| Helper.Instance.Initialize(); | ||
|
|
||
| var ruleOrder = new string[] |
There was a problem hiding this comment.
note sure if .net optimises that anyway but we could use a constant for this
bergmeister
reviewed
Jan 16, 2020
|
|
||
| Range updatedRange; | ||
| bool fixesWereApplied; | ||
| text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied); |
There was a problem hiding this comment.
the above bool fixesWereApplied line can be inlined just by using out bool fixesWereApplied here. Same for the Range parameter
bergmeister
reviewed
Jan 16, 2020
| /// PSUseConsistentWhitespace | ||
| /// PSUseConsistentIndentation | ||
| /// PSAlignAssignmentStatement | ||
| /// PSUseCorrectCasing |
There was a problem hiding this comment.
the exact list of those rules could change, I'd take them out of the comment therefore. Same for other comments
bergmeister
reviewed
Jan 16, 2020
| /// <summary> | ||
| /// The encapsulated results of the analyzer | ||
| /// </summary> | ||
| public class AnalyzerResult |
There was a problem hiding this comment.
please move each class/enum into its own file if it is public
bergmeister
reviewed
Jan 16, 2020
| 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.
I don't think this change is needed?
bergmeister
reviewed
Jan 16, 2020
| { | ||
| // 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.
variable declaration can be inlined: out Dictionary<string, object> settingDictionary, also in a few lines below
| <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.
Suggested change
| <licenseUrl>https://github.com/PowerShell/PowerShell/blob/master/LICENSE.txt</licenseUrl> | |
| <licenseUrl>https://github.com/PowerShell/PSScriptAnalyzer/blob/master/LICENSE</licenseUrl> |
|
Close and re-open to re-trigger CI |
|
I'm going to close this as it's not needed at the moment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
This 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
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.