FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add the AvoidExclaimOperator rule to warn about the use of the ! negation operator. Fixes #1826 by liamjpeters · Pull Request #1922 · PowerShell/PSScriptAnalyzer · GitHub

Add the AvoidExclaimOperator rule to warn about the use of the ! negation operator. Fixes #1826 - #1922

Merged
Christoph Bergmeister (bergmeister) merged 7 commits into
PowerShell:masterfrom
liamjpeters:master
Sep 6, 2023
Merged

Add the AvoidExclaimOperator rule to warn about the use of the ! negation operator. Fixes #1826#1922
Christoph Bergmeister (bergmeister) merged 7 commits into
PowerShell:masterfrom
liamjpeters:master

Conversation

Liam Peters (liamjpeters) commented Jun 14, 2023
edited
Loading

Copy link
Copy Markdown
Contributor

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 #1826

PR Checklist

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Liam Peters (liamjpeters) changed the title WIP: Add the AvoidExclamationPointOperator rule to warn about the use of the exclamation point negation operator. Fixes #1826 Add the AvoidExclamationPointOperator rule to warn about the use of the exclamation point negation operator. Fixes #1826 Jun 14, 2023
Liam Peters (liamjpeters) marked this pull request as ready for review June 14, 2023 11:56
Liam Peters (liamjpeters) marked this pull request as draft June 16, 2023 13:29
Liam Peters (liamjpeters) marked this pull request as ready for review June 16, 2023 13:29

Copy link
Copy Markdown
Contributor Author

Christoph Bergmeister (@bergmeister) - 👋 Can I get a review please? Sorry I didn't see a way to add a reviewer myself 😊 Thanks!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Rules/AvoidExclamationPointOperator.cs Outdated
Comment thread Rules/AvoidExclamationPointOperator.cs Outdated
Comment thread Rules/Strings.resx Outdated
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>
Liam Peters (liamjpeters) changed the title Add the AvoidExclamationPointOperator rule to warn about the use of the exclamation point negation operator. Fixes #1826 Add the AvoidExclaimOperator rule to warn about the use of the ! negation operator. Fixes #1826 Jun 18, 2023

Copy link
Copy Markdown
Contributor Author

Renamed rule from AvoidExclamationPointOperator to AvoidExclaimOperator

Copy link
Copy Markdown
Contributor Author

Christoph Bergmeister (@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?

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Collaborator

Sorry for the delay Liam Peters (@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.
There's actually one thing you could help with, could you run PSSA with this rule enabled against a more complex script like this one as a one off and check there are no exceptions https://github.com/PowerShell/PowerShell/blob/master/build.psm1
Then happy to merge, the rest of the code looks good to me :-)

Copy link
Copy Markdown
Contributor Author

Christoph Bergmeister (@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 -not and ! so is a good candidate. Using a text search there are 33 instances of the "!" character, 9 of them are within strings or comments (leaving 24 being used as negation).

PSSA with the rule enabled correctly flags the 24 negation instances.

Calling Invoke-ScriptAnalyzer with -Fix correctly changes the 24 negation instances.

Anything else you can think I should check - please let me know.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid exclamation point operator.

2 participants


Back | FazBrowse Home | New Git URL