PowerShell / vscode-powershell

Provides PowerShell language and debugging support for Visual Studio Code
https://marketplace.visualstudio.com/items/ms-vscode.PowerShell
MIT License
1.72k stars 492 forks source link

Add option to specify paths for ScriptAnalyzer-evaluation #3048

Open fflaten opened 4 years ago

fflaten commented 4 years ago

Summary of the new feature

This extension should include a setting to specify which paths to run Script Analysis against. Currently the extension reads the PSScriptAnalyzer.psd1-file in workspace root (if not modified) and executes PSSA against all files recursively.

Let's say you only want to run the rules against src/ and tool/. AFAIK this can't be configured in the extension nor PSScriptAnalyzer.psd1, which can result in many unnecessary problems and warnings in folders without the same requirements to follow best practices.

Proposed technical implementation details (optional)

Invoke-ScriptAnalyzer solves this problem by requiring a Path or ScriptDefinition parameter. As the extension uses the latter option, then the logic and paths-setting would have to be part of the Script Analysis-part of vscode-powershell + PowerShellEditorServices.

rjmholt commented 4 years ago

So I think adding a configuration like this to PSScriptAnalyzer's settings file makes sense, and for that I'd ask you to open a feature request issue in the PSScriptAnalyzer repo.

On the VSCode extension front, I want to understand the scenario a bit better.

You've said in the issue description:

Currently the extension reads the PSScriptAnalyzer.psd1-file in workspace root (if not modified) and executes PSSA against all files recursively.

Is that what you're observing?

Currently diagnostics are meant to be triggered only when a file is opened or edited, and analysis is only run on those files.

So if you're seeing the VSCode extension do something different, that's probably a bug.

Given that that's the intended behaviour, a configuration to exclude files just from script analysis is something we wouldn't really look to add unless there's a particular scenario you're trying to address.

Could you elaborate a bit on your scenario, what you're seeing and what the current vs ideal behaviour is?

fflaten commented 4 years ago

Yes, it only analyzes open files - but it analyzes any open powershell-file in the workspace. There's currently no way to limit vscode-powershell/PSES analysis to specific folders.

We don't need the same quality checks in less critical-code like /tst/, /tools/ etc. However /src/ which contains published code should always be analyzed. At the moment it's all or none. Example scenario: pester/pester#1761

How would adding the setting to PSScriptAnalyzer.psd1 solve this issue? vscode-powershell/PSES uses Invoke-ScriptAnalyzer -ScriptDefintion ... without providing the path of the current file to PSSA. The decision to auto-analyze the current file or not belongs to the extension/PSES. Am I missing something?

simonsabin commented 4 years ago

Would it be better to have the extension look for psd1 in each folder. That way you can specify different rules for different folders. That also then allows you to match the Invoke when you specify a path to analyse and the path to the psd1. Having it in settings feels a bit limiting

fflaten commented 4 years ago

That way you can specify different rules for different folders.

I like it. That was gonna be my next feature request, but I'd probably suggest extending the settings.json to support of multiple PSSA settings-files, each with their own path-settings.

Using a new psd1-file in each folder could easily lead to duplicate files. In my include/exclude folder-scenario you'd also have to create a new psd1 in every folder where PSES should not analyze which feels a bit messy. Personally I'd like to store a couple psd1-files somewhere and just reuse those for x folders each.

PSSA solves this problem by letting you run invoke-scriptanalyzer using a specific psd1 against different -path values. It's the way PSES currently implements PSSA, using -ScriptDefinition, that limits us atm. So why introduce many duplicate files in a repo to solve a editor-specific issue, especially when the editor already uses settings.json for other PSSA settings?

This could possibly be delayed and fixed as part of the PSSA2-rewrite since better integration-support for editors is one of the suggested features for the major release. 🙂

SydneyhSmith commented 4 years ago

Thanks for the clarifications, one more question--are you seeing performance impacts or other issues with the open files that you would not like to be utilized, or is the issue more along the issue of not wanting to see squiggles/ or other problems? (would be great to better understand why you dont want the files to be analyzed)...thanks!

fflaten commented 4 years ago

Currently to avoid unnecessary warnings/squiggles. Haven't seen any performance impacts (yet). Our files are probably to small for that to happen.

Can't speak for others though. I have seen a couple of earlier issues about excluding folders as well.

dbaileyut commented 1 year ago

The GitHub Copilot completions panel has a ton of errors detected when it's open and I would love to exclude those at the user settings level rather than per folder or workspace but still have PowerShell syntax highlighting.

image

Pxtl commented 3 weeks ago

I hit F12 on a call to an external referenced module (that I had put in a /myproject/packages dir) and it listed a zillion problems, so I'd appreciate the ability to have some kind of .analyzerignore folder concept. Or have it just ignore the files I've already .gitignored (since I've already .gitignored the packages folder), but I appreciate that's kind of presumptuous.

Also I didn't realize it only runs on open files. That's different from other tools, is there a toggle to change that?

Pxtl commented 3 weeks ago

So I think adding a configuration like this to PSScriptAnalyzer's settings file makes sense, and for that I'd ask you to open a feature request issue in the PSScriptAnalyzer repo.

They said to open this issue here.

https://github.com/PowerShell/PSScriptAnalyzer/issues/561#issuecomment-552165652

JustinGrote commented 3 weeks ago

@pxtl it requires contributions in both for the ideal solution. I've noted my thoughts in the PSScriptAnalyzer issue.