Skip to content
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

Avoid exclamation point operator. #1826

Closed
sarahelsaig opened this issue Aug 5, 2022 · 17 comments · Fixed by #1922
Closed

Avoid exclamation point operator. #1826

sarahelsaig opened this issue Aug 5, 2022 · 17 comments · Fixed by #1922

Comments

@sarahelsaig
Copy link

Summary of the new feature

While not technically an alias, ! behaves like an alias to the -not operator. Even "about_Logical_Operators" only mentions it in passing as "Same as -not" Considering that PSScriptAnalyzer has other rules against aliases, and that ! looks different to pretty much all other operator, I think it should also be warned against.

What is the latest version of PSScriptAnalyzer at the point of writing

1.20

@essentialexch
Copy link

We also have unary "-" and unary "++". I use "!" far more often than I use "-not".

And outside of the unary space, we have ternary and null-coalescing operators.

I would disable such a rule and personally don't like the idea.

@sarahelsaig
Copy link
Author

I can sympathize because there are aliases I like to use and whitelist. I'm fine with a rule that's not enabled by default, however -not is the official negation operator so it would be good and correct to allow enforcing it.

@MartinGC94
Copy link

Maybe I'm just weird but I don't like using -Not because it's the only "named" operator you place at the start of a statement instead of the middle or end of one. ! fits in with all the other operators you use at the start of statements like . and &.
The user experience of using -not also isn't that great because you can't tab complete it but that could be fixed.

@sarahelsaig
Copy link
Author

On the other hand all boolean operators are "named" so it makes sense to me that way. Either way, you should be able to enforce consistent operator usage.

@essentialexch
Copy link

On the other hand all boolean operators are "named" so it makes sense to me that way. Either way, you should be able to enforce consistent operator usage.

Nothing forces you to use -not. You can always reverse the logic of your conditional to avoid it, if you don't like it.

@essentialexch
Copy link

Maybe I'm just weird but I don't like using -Not because it's the only "named" operator you place at the start of a statement instead of the middle or end of one.

-join can also be used as a standalone prefix operator.

PS C:\> $a = 'a', 'b', 'c', 'd'
PS C:\> -join $a
abcd

@sarahelsaig
Copy link
Author

Nothing forces you to use -not.

The whole point of an analyzer is to enforce consistent practices in the organization.

@essentialexch
Copy link

The whole point behind PSScriptAnalyzer is to indicate parts of scripts which do not follow best practices. Which is why we are having the discussion as to whether your suggestion is a best practice.

PSScriptAnalyzer is a static code checker for PowerShell modules and scripts. PSScriptAnalyzer checks the quality of PowerShell code by running a set of rules. The rules are based on PowerShell best practices identified by PowerShell Team and the community.

from https://docs.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/overview?view=ps-modules

So far, only three people have engaged in the discussion. Two of them like ! and one doesn't. Hopefully a bunch more people will engage.

@Piedone
Copy link

Piedone commented Aug 17, 2022

I write some PS code, but more I review all kinds of code, and oversee a development team. While I don't necessarily have a preference for either syntax in this particular case, I'd like my team to settle on one of them, use it consistently and have automation in place to enforce this (and all kinds of such matters). We do the same thing with the static code analysis of C#, JS, SCSS too.

@JustinGrote
Copy link

I am in the -not camp, despite it being a terrible syntax in if statements. I would definitely turn on this rule on my project styles but this is one of those where there isn't a predominant style unfortunately. Similar to $_ vs $psitem.

As such its a great idea for a rule but should be opt-in I think.

@bjompen
Copy link

bjompen commented Aug 18, 2022

Second Justins opinon. A great idea and I would absolutely turn it on, but make it opt-in. Having it as a native rule makes it easier to enforce instead of requiring writing your own rules for standards.

@jhoneill
Copy link

Also with @JustinGrote .

If we had only one of ! or -not would we say
the boolean operators are -and -not -or and -xor
or
the boolean operators are -and -! -or and -xor

I know some people like ! and would like to have && and || too, and perhaps == instead of -eq; but I think -not is better style, but I don't think I will be able to persuade anyone who has an established preference for !

I think I must see hundreds or thousands of uses of $_ for each $psitem so there is a predominant style for that,
but for -not vs ! the split is nearer 50:50 - trying to get people to give up either of those through style rules is probably just going to annoy them without getting them to change.

The one case for having a rule is when contributing to a project. If it is my project I want -not throughout and if a rule means contributors don't submit code I need to change, that's a win. If I submit to someone else's project and they use ! throughout then a rule that means my submissions don't need changing is also a win. I'm not sure that having both as options is practical.

@bergmeister
Copy link
Collaborator

I think there is no right and wrong since it's just about readability. PSScriptAnalyzer is not just a code analyser but also a formatter and this is the perfect scenario. I suggest to create a new formatting rule (which can either be run by Invoke-Formatter or by Invoke-ScriptAnalyzer), that lets user highlight or format the code to THEIR preference. I would not turn on the rule by default, neither in PSSA nor in the VS-Code extension but I would add a codeformatting setting for the VS-Code extension. This way, people can be optionally informed of violations of their preferred style and have the ability to have it auto-fixed (both either via command line or in the editor). Would everyone be happy with that?

@Piedone
Copy link

Piedone commented Aug 30, 2022

Yes, sounds perfect.

@essentialexch
Copy link

That's excellent.

@jhoneill
Copy link

@bergmeister Yes, that sounds ideal to me

bergmeister added a commit that referenced this issue Sep 6, 2023
…tion operator. Fixes #1826 (#1922)

* Added the AvoidExclamationPointOperator rule to warn about the use of the negation operator !. Fixes #1826

* Updated license header of Rules/AvoidExclamationPointOperator.cs

* Updated the description of the rule in docs

Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>

* Fix alignment

Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>

* Update Rules/Strings.resx

Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>

* Renamed rule from AvoidExclamationPointOperator to AvoidExclaimOperator

* Added comment explanation of the replacement suggestion logic and renamed loop variable for clarity

---------

Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>
@Piedone
Copy link

Piedone commented Sep 6, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment