GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@JamesWTruher

@JamesWTruher James Truher (JamesWTruher) commented Oct 29, 2019

Copy link
Copy Markdown
Contributor

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
James Truher (JamesWTruher) added 30 commits September 12, 2019 14:27
@JamesWTruher
add sensible defaults code
a6f77d3
@JamesWTruher
Add hosted analyzer class
499a62a
@JamesWTruher
more hosting updates
1b7b9b1
@JamesWTruher
Formatter seems to be working
9e5cd87
Had to make the initialization of the analyzer optional as it was clobbering the settings that had been created in the formatter
@JamesWTruher
change calls to analyzer to be more like the cmdlet
8c0b30b
@JamesWTruher
checkpoint
e5e31af
Hosting seems to work, starting to work on nuget packaging
@JamesWTruher
Since RunspacePools are disposable, implement IDisposable for HostedA…
2de9d8f
…nalyzer, CommandCache, and Helper
@JamesWTruher
add another supported library
1034af8
@JamesWTruher
Remove IDisposable in Helper as we no longer create a runspace pool
795b68c
@JamesWTruher
make helper disposable to remove all vestiges of runspaces
a71ac90
@JamesWTruher
add sensible defaults code
65aae15
@JamesWTruher
Add hosted analyzer class
567f530
@JamesWTruher
more hosting updates
8d7d2a6
@JamesWTruher
Formatter seems to be working
73fec1b
Had to make the initialization of the analyzer optional as it was clobbering the settings that had been created in the formatter
@JamesWTruher
change calls to analyzer to be more like the cmdlet
874628a
@JamesWTruher
checkpoint
67dc945
Hosting seems to work, starting to work on nuget packaging
@JamesWTruher
Since RunspacePools are disposable, implement IDisposable for HostedA…
dd5b919
…nalyzer, CommandCache, and Helper
@JamesWTruher
add another supported library
01f7957
@JamesWTruher
Remove IDisposable in Helper as we no longer create a runspace pool
ed3436b
@JamesWTruher
make helper disposable to remove all vestiges of runspaces
db5c2f7
@JamesWTruher
Merge branch 'hosting' of https://github.com/JamesWTruher/PSScriptAna…
1b85759
…lyzer into hosting
@JamesWTruher
Add locking to hosted analyzer methods which change analyzer state
a3d58a8
@JamesWTruher
Add simple locking to hosted analyzer apis
b38e35d
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.
@JamesWTruher
Change hostedanalyzer fields to properties
b2e76ee
@JamesWTruher
Remove dispose of Helper.Instance.
ba88ff8
Disposing the helper seems to cause downstream problems.
Add async calls to the analyzer
@JamesWTruher
Add an overload for Fix which takes a settings object
7610d49
Add a simple tostring method to the HostedAnalyzer
@JamesWTruher
Adding tests for HostedAnalyzer
e43d72b
@JamesWTruher
fix ToString
c969ead
@JamesWTruher
Add C# tests for the hosted analyzer
c081444
This will check to be sure that we can actually using the nupgk in a project
update the version to 1.18.4
@JamesWTruher
Fix up reference assembly build and when it should be built
0dcb55d
James Truher (JamesWTruher) added 10 commits November 18, 2019 15:44
@JamesWTruher
Update version to 1.18.4 for signing files in hosting release
3e9d242
@JamesWTruher
Add reference build to build All
24533f3
@JamesWTruher
global.json needed in directory for build
bbb5d75
@JamesWTruher
Add .nupkg to be signed
00a73e3
@JamesWTruher
Add support for format with settings and range
c744561
Add tests for the api and in the reference assembly
@JamesWTruher
Add a couple of new apis that are needed by Editor Services
909f62a
Change the apis which allow adding settings to return the settings object so they can be chained
@JamesWTruher
Update reference library to include changes in previous hosted analyzer
3f1debe
@JamesWTruher
Make Fix and Format similar from a signature basis
798f8b2
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
@JamesWTruher
Merge branch 'master' into hosting
1e8c7d8
@JamesWTruher
match sdk version with base project
88f379c
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes
Comment thread Reference/global.json
@@ -0,0 +1,5 @@
{
"sdk": {
"version": "2.2.402"

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bergmeister Christoph Bergmeister (bergmeister) changed the title WIP: A new hosting model which is easier for Editor Services and other script analyzer hosting A new hosting model which is easier for Editor Services and other script analyzer hosting Jan 16, 2020
@bergmeister
Merge branch 'master' into hosting
28501a6
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes
Comment thread Engine/Formatter.cs
Helper.Instance = new Helper(runspace.SessionStateProxy.InvokeCommand, writer);
Helper.Instance.Initialize();

var ruleOrder = new string[]

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes
Comment thread Engine/Formatter.cs

Range updatedRange;
bool fixesWereApplied;
text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied);

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes
Comment thread Engine/HostedAnalyzer.cs
/// PSUseConsistentWhitespace
/// PSUseConsistentIndentation
/// PSAlignAssignmentStatement
/// PSUseCorrectCasing

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes
Comment thread Engine/HostedAnalyzer.cs
/// <summary>
/// The encapsulated results of the analyzer
/// </summary>
public class AnalyzerResult

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes
Comment thread Engine/Settings.cs
public IEnumerable<string> ExcludeRules => excludeRules;
public IEnumerable<string> Severities => severities;
public IEnumerable<string> CustomRulePath => customRulePath;
public List<string> IncludeRules => includeRules;

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jan 16, 2020
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Engine/Settings.cs
{
// 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))

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Reference/HostedScriptAnalyzer.nuspec
<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>

@bergmeister Christoph Bergmeister (bergmeister) Jan 16, 2020

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bergmeister
Christoph Bergmeister (bergmeister) requested a review from Rob Holt (rjmholt) January 16, 2020 21:39
@bergmeister Christoph Bergmeister (bergmeister) added the Area - Build label Jan 16, 2020
James Truher (JamesWTruher) added 3 commits January 28, 2020 12:17
@JamesWTruher
Add documentation for hosted analyzer
4e6c2ae
@JamesWTruher
Merge branch 'hosting' of https://github.com/JamesWTruher/PSScriptAna…
f65bb3e
…lyzer into hosting
@JamesWTruher
Merge branch 'master' into hosting
43af91c
@bergmeister

Christoph Bergmeister (bergmeister) commented Feb 5, 2020

Copy link
Copy Markdown
Collaborator

Close and re-open to re-trigger CI

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bergmeister Christoph Bergmeister (bergmeister) closed this Feb 5, 2020
@bergmeister Christoph Bergmeister (bergmeister) reopened this Feb 5, 2020
@JamesWTruher
Merge branch 'master' into hosting
2afcc2a
@JamesWTruher

James Truher (JamesWTruher) commented Apr 21, 2020

Copy link
Copy Markdown
Contributor Author

I'm going to close this as it's not needed at the moment.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JamesWTruher James Truher (JamesWTruher) closed this Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergmeister Christoph Bergmeister (bergmeister) bergmeister left review comments
@rjmholt Rob Holt (rjmholt) Awaiting requested review from rjmholt

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Area - Build

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@JamesWTruher @bergmeister
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.