Allow the user to customize PSScriptAnalyzer settings

[rkeithhill]

Here are the parameters to Invoke-ScriptAnalyzer:

Name                   Aliases      Position Mandatory Pipeline ByName Provider        Type
----                   -------      -------- --------- -------- ------ --------        ----
CustomizedRulePath     {C*}         Named    False     False    False  All             String[]
ExcludeRule            {Ex*}        Named    False     False    False  All             String[]
IncludeRule            {Inc*}       Named    False     False    False  All             String[]
Path                   {PSPath, ... 0        True      False    False  All             String
Profile                {Pr*}        Named    False     False    False  All             String
Recurse                {R*}         Named    False     False    False  All             SwitchParameter
Severity               {Se*}        Named    False     False    False  All             String[]
SuppressedOnly         {Su*}        Named    False     False    False  All             SwitchParameter

There are a number of items that users may want to customize. Simple things like IncludeRule, ExcludeRule and Severity. Profile string is however a path to a profile file. This would cover most individual parameters of Invoke-ScriptAnalyzer. The contents of this file look like this:

@{
    Severity='Warning','Information'
    IncludeRules=@( 'PSAvoidUsingPositionalParameters',
                    'PSAvoidUsingInternalURLs'
                    'PSAvoidUninitializedVariable')
    ExcludeRules=@('PSAvoidUsingCmdletAliases')
}

I think this profile file will tend to be unique to a workspace. We could certainly allow the user to set a "global" scriptAnalysisProfilePath in user preferences but we would need a way to override on a per-workspace basis. In fact, if we only provided one it should be the per-workspace profile file.

So for per-workspace profile, we could always look for a certain filename (by convention) perhaps ${workspaceRoot}\ScriptAnalyzerProfile.psd1 or we provide some way the user can supply a per-workspace path to the profile. I'm not sure how to do that other than to hijack launch.json which doesn't seem right. Perhaps we need something like project.json? Thoughts?

[daviwil]

This requirement was previously discussed in #24 but maybe it's better to start again in clean issue.

Originally I had considered achieving this by adding a JSON-based Script Analyzer configuration in the user's VS Code settings file(s), but I think just pointing to a Script Analyzer profile file is a much better approach (and would work across different editors). No reason for us to reinvent basically the same configuration format in JSON. psd1 will work well enough for our users.

Here's how the profile path setting would work in practice:

• If the user has not specified a Script Analyzer profile path setting, we will default to a profile that we'll include with the extension. Currently I've been hardcoding the profile info into Editor Services but we'll need to switch to a file that contains our default ruleset.
• If the user has specified a profile path in their user settings, it will be used.
• If the user has specified a profile path in their workspace settings, VS Code will override whatever the user entered in their user settings or our default if the user never set a global profile path.

I believe we need to provide a Script Analyzer profile path setting instead of finding one based on convention. The reason for this is because I'm planning to decouple the usage of Script Analyzer so that we're invoking it as a PowerShell cmdlet instead of taking a direct dependency in the PSES code (as we currently do).

I'm also planning to wire it up through the upcoming PSES extensibility model so we ultimately won't be able to do any special-case behavior for Script Analyzer anymore (like determining a conventional profile path in the extension code). If we want to pass a profile path to Script Analyzer in this new model, it'll have to be done through whatever extension settings model I come up with. I'm hoping to have some time next week to start sketching out a design for this in code so that we can see how it could work.

Does that make sense? Probably a more complicated answer than you were expecting :)

[rkeithhill]

That all sounds good to me but I'm a little fuzzy on this:

If the user has specified a profile path in their workspace settings

So the extension settings model would need to support per-workspace settings which implies each workspace would need a setttings file of some sort like a project.json file (or some such name). I could see a file like this containing this at the moment:

{
    "scriptAnalysisProfilePath": "${workspaceRoot}/ScriptAnalyzerProfile.psd1"
}

Then again, maybe it should be (as you mention) project.psd1:

@{
    ScriptAnalysisProfilePath = './ScriptAnalzyerProfile.psd1'
}

where . is the ${workspaceRoot} dir.

What else do you think we'd put in a workspace settings file?

BTW I looked at #24 - not real sure how that applies??

[daviwil]

Whoops, I meant #28.

VS Code already has a settings override model for global vs workspace settings. For instance if you set the powershell.scriptAnalysis.enable to false globally through File -> Preferences -> User Settings, you can override that setting to true in the workspace's .vscode\settings.json file. Any setting that is specified there will override the same one in their global settings.

For this to work with the extension model's settings capabilities in the context of VS Code, we'd need some way to dynamically add new settings that can be displayed to the user in the different settings screens. Which reminds me, I need to file a bug for that... Since this capability isn't already there we may need a more hardcoded solution in the short term once the extensibility model starts to materialize.

[rkeithhill]

.vscode\settings.json

Ah that is the bit I was missing. So in .vscode\settings.json you could have a scriptAnalysisProfilePath setting that would override the user/global setting. Cool. Sounds like Examples\.vscode is about to get a new file. :-)

[daviwil]

Yep! For other editors we'd need to figure out the editor-specific way to expose configuration values. There might be some value to a PSES-specific configuration model but that might be a little strange since most editors have their own way of doing things. We may need to re-evaluate that in the future.

[rkeithhill]

RE #28 - yeah I think with that issue the focus is more on ensuring the default ruleset was more reasonable. I "think" you've accomplished that and could consider closing that issue with a pointer to this issue as a continuation of the discussion about user customization.

[daviwil]

Good point, closed it.

[rkeithhill]

Ugh, the method I want to use in Script Analyzer is private: :-(

        private bool ParseProfileString(string profile, PathIntrinsics path, IOutputWriter writer,
            List<string> severityList, List<string> includeRuleList, List<string> excludeRuleList) { ... }

What do you think the chances are of getting PSScriptAnalyzer to add an ScriptAnalyzer public Initialize override like this:

        public void Initialize(
            Runspace runspace, 
            IOutputWriter outputWriter, 
            string[] customizedRulePath = null,             
            string settingsPath = null,
            bool includeDefaultRules = false,
            bool suppressedOnly = false)
        {
        }

or make the ParseProfileString method public. However from the P.O.V. of their API I think there should be an Initialize override that takes a settings path.

[daviwil]

I'd say adding another optional param to the Initialize method is the way to go here. That method is the entry point I added to Script Analyzer for use as a .NET API. If you want to send a pull request to the repo with that change, I'll back you up.

[rkeithhill]

OK got something that works. We might want to review this at some point. I could publish it to my forks of vscode-powershell and PSES and have you take a look. Or if you're up for it, do a review over Skype. I definitely have some questions about how to handle resolution of the scriptAnalysis.settingsPath from relative to absolute especially in the face of users update their preferences at any point in time.

[daviwil]

I'm happy to do the review whichever way works best. I'll be home working on stuff today so we can jump on Skype in the early/mid afternoon if you like.

[rkeithhill]

OK, I've published this to my repo if you want to mess around with it. The branches are:

https://github.com/rkeithhill/vscode-powershell/tree/rkeithhill/is122-scriptAnalysisProfilePath
https://github.com/rkeithhill/powerShellEditorServices/tree/rkeithhill/scriptAnalysisProfilePath

The PSES branch didn't get the updated ScriptAnalyzer.cs file but you can grab the diff from:
PowerShell/PSScriptAnalyzer#478

[rkeithhill]

@daviwil Do you know how to set a process environment variable in the extension's main.ts file? If we could set a few environment variables like PSES_WORKSPACEROOT and PSES_EXTENSIONROOT then we could allow users to specify a script analyzer settings path like %PSES_WORKSPACEROOT%\mysettings.psd1 and then on the language server side, we can expand those env vars.

[rkeithhill]

So we can set an env var for the extension root dir in main.ts:

    process.env['PSES_EXTENSIONROOT'] = path.resolve(context.extensionPath);

And we can set the PSES_WORKSPACEROOT in the editor host since that path gets passed via the initializeRequest. So we could make this work by A) disallowing relative path because we don't know what a relative path should be relative to (extension install dir or workspaceRoot) and B) using an env var in the path e.g. %PSES_WORKSPACEROOT%\settings.psd1.

[rkeithhill]

OTOH we could allow the user to put in ${workspaceRoot} and expand that value ourselves? That would at least feel familiar and if the VSCode started doing this for us, we could remove that code.

[daviwil]

Yeah, I think it's ultimately more likely that they'll make their own variables (like workspaceRoot) work. I saw in another issue that they were thinking system environment variables aren't a good idea because the variable string is different on various platforms.

[rkeithhill]

So should we punt on this feature until VSCode makes this work for us? Perhaps this feature should be released only when the "use external PSScriptAnalyzer" feature is ready?

FWIW I'm not as confident the VSCode team will address this anytime soon - see microsoft/vscode#5089 which was closed.

[daviwil]

Would there be value in shipping the feature with the temporary limitation that the settings file must always be either an absolute path or relative to the workspace path (without using $workspaceRoot)?

When the language server starts up, we get the workspace path as part of the initialize request and it gets stored in EditorSession.Workspace.WorkspacePath. For now we could just use that path to build the path to the workspace's PSSA settings file if it's not already an absolute path.

For the default case, I'd say our VS Code setting should just be an empty string. When the server gets the setting as empty string, we can try to use our default settings as he have been so far.

What do you think?

[rkeithhill]

I'm working on it.

[rkeithhill]

OK I have two PRs pending that enable your scenario.