-
Notifications
You must be signed in to change notification settings - Fork 403
Add AvoidReservedWordsAsFunctionNames Rule #2128
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 AvoidReservedWordsAsFunctionNames Rule #2128
Conversation
|
Just as a note... I did attempt to use reflection rather than a hard-coded list of keywords. The keyword list is in the Tokenizer. public static HashSet<string> GetReservedWordsFromTokenizer()
{
// Get the assembly containing Tokenizer
var asm = typeof(Token).Assembly;
// Get the internal Tokenizer type
var tokenizerType = asm.GetType("System.Management.Automation.Language.Tokenizer");
// Get the private static readonly field s_keywordTokenKind
var field = tokenizerType.GetField("s_keywordTokenKind", BindingFlags.NonPublic | BindingFlags.Static);
// Get the TokenKind[] value
var tokenKinds = field.GetValue(null) as Array;
var reservedWords = new HashSet<string>(
tokenKinds.Cast<Enum>().Select(tk => tk.ToString()),
StringComparer.OrdinalIgnoreCase
);
return reservedWords;
}With similar on the PowerShell side for the tests. This worked fine in PS7 but I can't seem to get it to work in PS5.1 - I can't access the |
That makes sense to me. We've run into similar issues in PSES because the visibility of some of them changed making them available through reflection in PS7 but still not in PS5.1. Oh well 🫠 |
6672621 to
1e5c1b2
Compare
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.
@SeeminglyScience can you take a look at this too? Seems pretty good to me and reasonable to add as a default.
…ed word, and reserved word with a letter missing
…st for good measure
Co-authored-by: Andy Jordan <2226434+andyleejordan@users.noreply.github.com>
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.
I'm good with this, just want @SeeminglyScience to double check. I'll send it his 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.
Definitely a useful rule, ty Liam! Couple of small changes
…bility Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>
assembly, base, command, hidden, in, inlinescript, interface, module, namespace, private, public, static
|
We did it, high-fives all round 🫸🫷 |
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.
LGTM!
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.
LGTM!
PR Summary
Adds a new rule that warns when reserved words are used as function names.
Fixes #2099
I wrote a blog post about putting this together 😀
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.