-
Notifications
You must be signed in to change notification settings - Fork 407
Add UseConsistentParameterSetName Rule #2124
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
base: main
Are you sure you want to change the base?
Add UseConsistentParameterSetName Rule #2124
Conversation
| 1. **Missing DefaultParameterSetName** - Warns when parameter sets are used but no default is specified | ||
| 2. **Multiple parameter declarations** - Detects when a parameter is declared multiple times in the same parameter set. This is ultimately a runtime exception - this check helps catch it sooner. | ||
| 3. **Case mismatch between DefaultParameterSetName and ParameterSetName** - Ensures consistent casing | ||
| 4. **Case mismatch between different ParameterSetName values** - Ensures all references to the same parameter set use identical casing | ||
| 5. **Parameter set names containing newlines** - Warns against using newline characters in parameter set names |
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.
We need to verify our understanding of parameter sets to review this better, we'll get back to it Liam!
|
@liamjpeters can you resolve merge conflict please so we can run CI? |
Done. If you approve the CI runs, I'll fix any issues that may have cropped up |
Thanks for quick turnaround, approved just now. You should be able to run the tests in your fork as well btw (at least that was the case when we were in Azure DevOps before we moved CI to GitHub). If not, let me know, we can look into enabling that generally or just for you as a regular contributor |
| | [UseCompatibleSyntax](./UseCompatibleSyntax.md) | Warning | No | Yes | | ||
| | [UseCompatibleTypes](./UseCompatibleTypes.md) | Warning | No | Yes | | ||
| | [UseConsistentIndentation](./UseConsistentIndentation.md) | Warning | No | Yes | | ||
| | [UseConsistentParameterSetName](./UseConsistentParameterSetName.md) | Warning | Yes | | |
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.
since it's a new rule and Andy's comment indicated we aren't 100% sure on everything, might be best to start with not enabled by default in first release and pending user feedback enable it by default later. My experience over the last years is that users find surprisingly many edge cases even in simple rules and had to learn this the hard way.
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.
ok cool. I'll definitely take that on board and learn from your hard-earned experience. I'll get this changed to an optional rule over the weekend.
The triggers for the CI are a push or PR to |
PR Summary
Add UseConsistentParameterSetName rule to detect potential parameter set issues
Parameter set names are case-sensitive in PowerShell (unlike most other elements), which can lead to runtime errors and unexpected behavior when not handled correctly.
The rule performs five key validations:
Fixes #396 with the exception of
ParameterSets that don't have at least one unique parameter.This won't help with Dynamic Parameters.
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.