-
Notifications
You must be signed in to change notification settings - Fork 382
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
Add the AvoidExclaimOperator rule to warn about the use of the ! negation operator. Fixes #1826 #1922
Conversation
@microsoft-github-policy-service agree |
@bergmeister - 👋 Can I get a review please? Sorry I didn't see a way to add a reviewer myself 😊 Thanks! |
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 overall, just a few comments. Not sure on the rule name though, it's definitely rather exclamation mark and not exclamation point. Maybe even as short as ExclaimOperator
, since that is the name of the token.
Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>
Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>
Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>
Renamed rule from |
…amed loop variable for clarity
@bergmeister - Thanks for your feedback so far - really helpful 😀. Was there anything else that needs to change for this to be considered for a merge? Anyone else I should be asking for a review from? |
Added James as well to review. Other than that I just want to test drive it a bit myself for some exploratory testing as the rule is disabled by default so wouldn't get exercised as part of existing test suite scenarios |
Any news on this? I don't really know what my expectations should be, as this is my first contribution to this project - so please feel free to tell me to cool my jets and just be more patient. I appreciate this PR doesn't introduce some killer feature that will be used by loads of people - but I'm excited for it and don't want the PR to be abandoned. Equally I'm excited to pick up other issues and open more PRs to improve the tool and my own c#, after testing the waters here. Is there anything I can do to help? |
Sorry for the delay @liamjpeters . It's not a bumper feature but because it can run on any PowerShell code, it's easy to miss a case that could result e.g. in a NullReferenceException. But the feature is not enabled by default, which is great to limit blast radius. |
@bergmeister - Good idea to run it on a more complex test file. I've run the existing and updated PSSA against the Powershell project's build file. No exceptions thrown and no difference in execution time (both take an average of 170ms on my machine over multiple runs - even with my additional rule enabled). Invoke-ScriptAnalyzer -Path .\build.psm1 -Settings @{Rules = @{ 'PSAvoidExclaimOperator' = @{ Enable = $true }}} The build file has a mix of usages of PSSA with the rule enabled correctly flags the 24 negation instances. Calling Anything else you can think I should check - please let me know. |
Add a rule to warn about using the exclamation mark (
!
) as the negation operator. Suggests using-not
instead.PR Summary
Add the PSAvoidExclaimOperator rule to warn about the use of
!
for negation. The rule is disabled by default as many people see this as a stylistic preference. Covered by tests .Fixes #1826PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.