Add UseConsistentCasing by Jaykul · Pull Request #1704 · PowerShell/PSScriptAnalyzer
Conversation
Add lowercase keyword (and operator) enforcement as a separate rule.
I was thinking about adding a check for the correct case of [Types] and [Attribute()]s, but I think that should probably be in the original rule (UseCorrectCasing), which begs the question of whether we should just add that (or all of this) to the existing rule...
Originally, I made a separate rule because I thought I might need to configure the type of case ("lowercase", "UPPERCASE", "CorrectCase") ... but I quickly decided all-caps is awful, and there is no "correct" case (the C# tokenizer code just capitalizes the first letter of each token, and the documentation is inconsistent about capitalization of keywords and operators).
Jaykul
marked this pull request as ready for review
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort, looks ok from a high level.
But we already have a UseCorrectCasing rule, which is used for cmdlet casing at the moment but I think we could just add CheckOperator and CheckKeyword as options to it and a new option for CheckCmdlet so that user can pick and choose what they prefer. Keeping it in one rule will make things easier to integrate the new settings into the VS-Code extension, which already has scaffolding for UseCorrectCasing. Also, if you wanted the rule to be usable by the formatter and the vs-code extension, you'd have to register it here.
WDYT @rjmholt
which begs the question of whether we should just add that (or all of this) to the existing rule...
we already have a UseCorrectCasing rule, which is used for cmdlet casing at the moment but I think we could just add CheckOperator and CheckKeyword as options to it and a new option for CheckCmdlet so that user can pick and choose what they prefer
Sorry, I meant to get back to this question earlier.
I think it's better all in one rule, because:
- We have an ongoing problem in PSScriptAnalyzer where rules that do specific things are given general names and then confusingly have competing rules next to them with a name that makes them hard to differentiate
- We also have the opposite problem where some rules have very specific names but philosophically should do more (for example
AvoidUsingInvokeExpressionshould also warn against[scriptblock]::Create()and maybe some other things) - We have a good story for rule configuration
- In this particular case, I think the desires to case cmdlets and to case keywords/operators correctly are cohesive and really do belong in one rule
- We unfortunately also have a bad story for formatting currently, so piggy-backing on an established formatting rule also provides a workaround there
All the code looks good and looks pretty portable at this stage, so hopefully it's just a case of sticking it inside of some conditional methods in UseCorrectCasing, rejigging the configuration and moving the tests over. But let me know if it's more complicated than that.
I'm quite happy to merge them. This was a safer PR just because if you didn't want it, I could take my code and go home 😉
I did add this one to the formatter (you can see in the tests that I verified that worked).
Any updates on this PR? Would be nice to have this feature in vscode so it autoformats my keywords properly.
Reminder @Jaykul that it's already next week 😂😂
Last chance @Jaykul as we are starting to wrap up for next release this month
It's been a long, long time...
There's so much I feel that I should say
But words can wait until some other day
I have seen that this PR has gotten stale a bit again. Is there anything that could be done to help complete it?
I came across this again, because I noticed that I lack an auto-formatter for casing in VS Code for my PowerShell scripts. Which is why it would be really cool to have.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If tests still pass, happy to finally merge it :-)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the changes include configuration options. We need to document that. I am OK with accepting the PR as is if someone will add a comment to the PR explaining the configurating with an example. I can update the docs based on that.
| It "Does not throw when correcting certain cmdlets (issue 1516)" { | ||
| $scriptDefinition = 'Get-Content;Test-Path;Get-ChildItem;Get-Content;Test-Path;Get-ChildItem' | ||
| $settings = @{ 'Rules' = @{ 'PSUseCorrectCasing' = @{ 'Enable' = $true } } } | ||
| $settings = @{ 'Rules' = @{ 'PSUseCorrectCasing' = @{ 'Enable' = $true; CheckCommands = $true; CheckKeywords = $true; CheckOperators = $true } } } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the settings for the rule
That's an interesting request, @sdwheeler -- do we have documentation somewhere about the options?
I mean, it's not generally in the docs ...
The settings are:
'PSUseCorrectCasing' = @{ Enable = $true CheckCommands = $true # Require the case of all commands to match their actual casing. CheckKeywords = $true # Require the case of all keywords to be lowercase. CheckOperators = $true # Require the case of all operators to be lowercase. }
The default should be all
$truethe way I wrote it in the example.
I understand the example shows them as all true. What is the default value if the user doesn't provide a configuration?
Sorry, I went back and reviewed the code again. I see where you set the default values.
@bergmeister thoughts on merging this and doing a 1.3 release?
@bergmeister thoughts on merging this and doing a 1.3 release?
@andyleejordan Yes to both.
I assume you meant 1.24 release though? Feel free to message me if you have question or want to plan it. I am on Slack and the PWG (PowerShell Working Group) Discord. At release time, we also sync our doc changes into msdocs repos, which is also a good time to check whether there were any changes in msdocs that weren't backported to PSSA (Sean usually does it but as it's manual, it's easy to omit some)
For consumers like PSES, we specify the rules and arguments explicitly and should set our own defaults. What I've usually done is add variable layer in PSES repo and in vscode-powershell repo, add new user setting(s) for it but don't enable them in next extension release so that people can try it and feedback before enabling by default.
Wiring in PSES is here: https://github.com/PowerShell/PowerShellEditorServices/blob/main/src/PowerShellEditorServices/Services/Workspace/LanguageServerSettings.cs
This was referenced
This was referenced