-
Notifications
You must be signed in to change notification settings - Fork 513
Create a choice list of PSSA rules #358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi @kapilmb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
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.
How does this interact with the PowerShell extension setting?
// Use a custom PowerShell Script Analyzer settings file for this workspace.
// Relative paths for this setting are always relative to the workspace root dir.
"powershell.scriptAnalysis.settingsPath": "PSScriptAnalyzerSettings.psd1"
This maintains consistency with the source file names in the project root.
@rkeithhill For now, it doesn't interact with the PSSA settings file - If the user supplies a settings file, it will not show any rule list. Otherwise, it will show a list of rules from a drop down to select from. |
But, in the near future this feature will take into consideration the data in the settings file and hence can provide an interface to update the settings file. |
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.
Looks like this must need an equivalent PR on the Editor Services side too?
|
||
constructor(options: ICheckboxOption[]) { | ||
this.options = options; | ||
this.confirm = figures.tick; |
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.
Instead of using the 'figures' package, it'd be more convenient to use the built-in icons that you can reference by a special string syntax. Here's an example: https://github.com/PowerShell/vscode-powershell/blob/master/src/session.ts#L391. Those string names are refer to the names of GitHub's Octicons. In this case you could just use $(check)
to achieve the same effect.
Here is the equivalent PR on EditorServices: PowerShell/PowerShellEditorServices#311 |
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.
Looks good now!
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.
Almost there! Just one more thing to improve.
} | ||
|
||
public setLanguageClient(languageclient: LanguageClient): void { | ||
this.languageClient = languageclient; |
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.
One more thing here: I think your request for getting the PSSA rules needs to be done in setLanguageClient. Since this method is called when the session is started, restarted, or switched to a new runtime, we'll need to re-query this list to make sure the list is up to date.
It makes sense to maintain the list of selected rules across sessions, though. This might require getting the list of rules and comparing it against the current list of rules that have been set so that you can set those rules on the new session. If this is too complicated to get done right now, we at least need to get the set of rules from the server when the LanguageClient changes.
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 current implementation doesn't required the client the maintain any rule state at all. The client retrieves the rule list whenever the user opens the quickpick menu and sends the updated list to the server. It does not maintain any state about the rule list.
So, if the LangauageClient changes and if the user opens the quickpick menu, it will get a new list from the server.
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.
Oh! OK, that makes sense. I should have paid closer attention :) cool, will merge this now!
No description provided.