Settings file creation and validation#2176
Settings file creation and validation#2176liamjpeters wants to merge 3 commits intoPowerShell:mainfrom
Conversation
…onfigurable rules
- Implemented `New-ScriptAnalyzerSettingsFile` cmdlet to create a new PSScriptAnalyzer settings file, with options for presets and overwriting existing files. - Added `Test-ScriptAnalyzerSettingsFile` cmdlet to validate settings files, checking for parseability, rule existence, and valid options. - Created comprehensive tests for both cmdlets to ensure functionality and error handling. - Updated module manifest to export the new cmdlets. - Added documentation for both cmdlets, including usage examples and parameter descriptions. - Enhanced error messages in the strings resource file for better clarity during validation failures.
- Update Helper.cs to return null for empty output paths instead of an empty array. - Add new error message for invalid option types in Strings.resx. - Extend tests for New-ScriptAnalyzerSettingsFile to check for new keys: CustomRulePath, IncludeDefaultRules, and RecurseCustomRulePath. - Modify Test-ScriptAnalyzerSettingsFile tests to validate output and error handling for various scenarios, including type mismatches and invalid values. - Improve documentation for New-ScriptAnalyzerSettingsFile and Test-ScriptAnalyzerSettingsFile to clarify behavior and parameters, including handling of custom rules and output format.
andyleejordan
left a comment
There was a problem hiding this comment.
Thanks Liam — I reviewed this with help from Claude Opus 4.7: Code quality is high (clean separation of formatting/validation/AST helpers, defensive null checks, proper ShouldProcess on New-ScriptAnalyzerSettingsFile), the new public surface is purely additive (new cmdlets, a new Options property on RuleInfo via a new constructor overload — existing constructors and consumers untouched), and test coverage is genuinely thorough on both happy and error paths.
A few thoughts before I approve:
- Maintenance cost is real but bounded. Once these ship they're public API; future rule additions will need their configurable options to round-trip cleanly through
RuleOptionInfo, andTest-ScriptAnalyzerSettingsFilebecomes coupled to the.psd1settings schema. The enum-discovery heuristic inRuleOptionInfo(the naming-convention match on the option's property type) is clever but silent on mismatch — worth either documenting the convention or considering an explicit attribute, so a future contributor doesn't accidentally ship a rule whosePossibleValuescome out empty. - Relationship to #2134. You've explicitly carved this off from the JSON-settings refactor, and as far as I can tell #2176 stands alone against the existing
.psd1path. If #2134 lands later, both new cmdlets will need format-aware updates — but that's expected and not a blocker here. - vscode-powershell connection. Have you seen vscode-powershell#5385? Users keep getting bitten by the extension's effective defaults differing from
Invoke-ScriptAnalyzer's defaults, and the workaround they're given is "author aPSScriptAnalyzerSettings.psd1by hand," which is exactly the cliff this PR makes shorter.New-ScriptAnalyzerSettingsFile -BaseOnPreset <…>plusTest-ScriptAnalyzerSettingsFiletogether give them a discoverable starting point and a way to confirm it parses. Worth calling out in the docs/examples? - @bergmeister — would value your read on this one before merging, particularly on (1) maintenance posture and on whether the
RuleOptionInfoheuristic is something we want to lock in or constrain.
Me: Overally I support merging this. As a new feature I'm less inclined to carefully pick through the whole implementation and will defer to you, especially given the thorough tests, but want to be certain you'll be around to help maintain it if people start using it.
— Drafted by Copilot (Claude Opus 4.7)
| `IncludeRules` list and populates the `Rules` section with all configurable properties, set to their | ||
| default values. Both modes also include `CustomRulePath`, `RecurseCustomRulePath`, and | ||
| `IncludeDefaultRules` keys with descriptive comments so the file is immediately ready for | ||
| customisation. |
There was a problem hiding this comment.
Our style guide dictates American spellings.
| customisation. | |
| customization. |
| customisation. | ||
|
|
||
| If a settings file already exists at the target path, the cmdlet emits a terminating error unless | ||
| the **Force** parameter is specified - in which case it is overwritten. |
There was a problem hiding this comment.
| the **Force** parameter is specified - in which case it is overwritten. | |
| the **Force** parameter is specified - in which case it's overwritten. |
| discovery in `Invoke-ScriptAnalyzer` picks it up when analysing scripts in the same directory. | ||
|
|
||
| Note: Relative paths in `CustomRulePath` are resolved from the caller's current working directory, | ||
| not from the location of the settings file. This matches `Invoke-ScriptAnalyzer` behaviour. |
There was a problem hiding this comment.
| not from the location of the settings file. This matches `Invoke-ScriptAnalyzer` behaviour. | |
| Relative paths in `CustomRulePath` are resolved from the caller's current working directory, | |
| not from the location of the settings file. This matches `Invoke-ScriptAnalyzer` behavior. |
PR Summary
Extracted and built upon the settings discoverability of my PR #2134 (omitting all of the JSON file format bits and settings refactoring).
New-ScriptAnalyzerSettingsFile, which generates complete settings files with everything set to it's default value.Test-ScriptAnalyzerSettingsFile, which validates a settings file (All rule names are valid, all settings valid etc).Get-ScriptAnalyzerRuleto include anOptionsproprety for configurable rules, including default value.ProcessCustomRulePathsEngine Helper to ignore an empty array ofCustomRulePaths.Motivation
It's not easy or convenient to find out what all the rules and rule settings available are. Your options are really to look at this repo, copy a preset/existing settings file, or look through all of the rules pages on MSLearn. Many people do not even know about settings files.
As a module developer, I want a way to discover what the available rules are and what options they have. I want an effortless way to add a settings file to my project that I can configure to my liking.
New-ScriptAnalyzerSettingsFileBy default this creates a ScriptAnalyzer settings file populated with all rule names and rule configuration (set to default values). The file has some comment help for each field.
The file is always named
PSScriptAnalyzerSettings.psd1so that automatic settings discovery inInvoke-ScriptAnalyzerpicks it up without any-Settingsparameter needed.Any rule configuration that is an enum - we attempt to list the possible values as a comment after the default value.
Example output of running
New-ScriptAnalyzerSettingsFilein this gistYou can optionally base the rule file on a preset. It takes the settings defined in the preset and normalises it all with the comments and all settings fields.
Example output of running
New-ScriptAnalyzerSettingsFile -BaseOnPreset CmdletDesignin this gist.Test-ScriptAnalyzerSettingsFileThis validates a settings file and tells you exactly what's wrong and where. It checks for:
IncludeRules,ExcludeRules, andRulesRulesthat aren't configurablebool,int,string,arrays)Kind = 'banana'when the valid values arespaceandtab)Each problem comes back as a
DiagnosticRecord, unless-Quietmode used - then just a boolean$true`$false` indicating whether the file is valid or not is returned.I debated for a while whether this should have just been a rule and after some chicken-or-egg thinking, decided it should be a separate cmdlet.
Get-ScriptAnalyzerRuleAdded an
Optionsproperty to the RuleInfo. I have not updated the format file, so it's not shown by default. I didn't know the implications of updating the format file.Running
Get-ScriptAnalyzerRule -Name PSUseConsistentIndentation | select -expand Optionsgets you:Successful test run in my fork.
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.