Add 'UseCorrectCasing' formatting rule for cmdlet/function name #1117

bergmeister · 2018-12-21T16:26:42Z

PR Summary

This is purely a code style rule, not a code analysis rule, i.e. it won't pester people running Invoke-scriptanalyzer unless they specify so in the settings file.
Fixes #1112
Unfortunately as part of doing a style rule the first time, I found there is some hard-coding in code of the rule names and I had to workaround a bug but at least I could boil it down and raise an issue for it here, I also have PR #1121 open for a fix, so I could remove the 1 line workaround in the test before we merge this.
At the moment, this new feature can only be used via Invoke-Formatter but later I plan to make PRs to PSES and the vscode-PowerShell extension to get this configured and enabled by default.

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…nalyzer into UseCorrectCasing

JamesWTruher

the real issue I have is the performance of newing up PowerShell n times. Is there a way we can avoid that. It may happen in other places, but we shouldn't perpetuate a bad performance experience.

JamesWTruher · 2019-01-14T19:50:52Z

Rules/UseCorrectCasing.cs

+                    continue;
+                }
+
+                using (var powershell = System.Management.Automation.PowerShell.Create())


I would be much happier to use a single instance of PowerShell rather than to create a new instance of PowerShell for each found command.
Also, what do we do about things which look like a command, but can't be found by Get-Command. Should we attempt to validate those?

I changed it to use the Helper.Instance.CommandInfo method for the moment because this method has commandinfo caching (but the disadvantage of this method is that it has uses a lock internally). I measured it before and after this change and could not measure a difference though, even compared to even with a 3rd version where I cached the PowerShell instance instead of always creating it... I also measured the difference between development and this branch and for a 600 line script file, I'd say it increased only a bit (from 370ms to 400ms), so I suspect there is something else that is the bottleneck. Formatting of a whole document is also not a task that a user would run frequently.
Would performance be improved if we used the async Execution APIs of PowerShell instead? Would it be acceptable to not enable this rule by default until we've found a better solution? Via the PR in the vscode extension we could run the rule by default for Invoke-Formatter but not with the default settings of the vscode extension.
I'm also working on a prototype in the background that speeds up PSSA by a factor of 10 in general by calling Get-Command in the Helper class once initially in the background to have a much richer cache, the only disadvantage is that Get-Command does not populate certain properties that are needed by other rules (I raised issue 8910 in the PowerShell/PowerShell repo for this), therefore until I figure out a solution to it, this is the performance bottleneck.

I reckon for the moment this is ok - it's something that I'll watch though.

JamesWTruher · 2019-01-14T19:57:29Z

Rules/UseCorrectCasing.cs

+
+                    var commandInfo = getCommand.Invoke<CommandInfo>().FirstOrDefault();
+                    if (commandInfo != null)
+                    {


What do we do about things which look like a command, but can't be found by Get-Command. Should we attempt to validate those? Is ignoring them as helpful as we could be? Could we suggest "get-zebra" is "Get-Zebra" (which could be done via simple regex)

Changing get-zebra to Get-Zebra of the command is a bit too assumptuous to me, this would be better suited for something like a configurable script-style rule.
The idea of this PR is to only add something small and simple where we are sure. I could do more, I could also fix the casing of parameter names but the idea is to add something small and minimum viable to have value to the user and based on feedback, invest more time in adding more functionality.

…nalyzer into UseCorrectCasing # Conflicts: # Rules/Strings.resx

…nalyzer into UseCorrectCasing

timoline · 2019-02-23T08:40:00Z

Why is this still blocked? This feature is very welcome.

bergmeister · 2019-02-23T10:34:48Z

Because of the performance impact due to the command lookup. I will work on that.
But more importantly I am also working on improving the engine in the background at the moment (I have a PoC that speeds it up by a factor of 10 but it needs more time until I can open a PR for it).
Maybe we could allow this PR if we ensure that the vscode extension does not enable this option by default

timoline · 2019-02-23T12:39:42Z

Because of the performance impact due to the command lookup. I will work on that.
But more importantly I am also working on improving the engine in the background at the moment (I have a PoC that speeds it up by a factor of 10 but it needs more time until I can open a PR for it).
Maybe we could allow this PR if we ensure that the vscode extension does not enable this option by default

ok great , I really appreciate your work on this.
thx

# Conflicts: # Tests/Engine/GetScriptAnalyzerRule.tests.ps1

rjmholt

This could possibly use var a little bit less (the main foreach loop requires the reader to do a lot of type inference), but the code looks good

…nalyzer into UseCorrectCasing # Conflicts: # Rules/Strings.resx

…rShell#1117) * first working prototype: Invoke-Formatter get-childitem * tweak for fully qualified cmdlets and add tests * add rule documentation and fix 3 tests as part of that * add resource strings * fix 1 test (severity) and docs test due to rule name * fix failing test using a workaround * fix tests * remove workaround in test that has been fixed upstream * cleanup and use helper method for getting cached cmdlet data

timoline · 2019-03-29T20:23:21Z

Hi,
works great for known functions/cmdlets, thx

But for new functions, it does not work (for me)

function get-caffee
{
    
}

get-caffee

rjmholt · 2019-05-24T18:19:38Z

@timoline I believe casing is based on the rule definition, so if you define the function in all lower case, the "correct" form for invocations later is that all lower case form. Try writing your function definition in PascalCase and see if the rule updates the invocation

timoline · 2019-05-25T04:09:18Z

@rjmholt
if I format the below code, only Get-Alias will be changed to correct casing, get-caffee will stay the same

function Get-Caffee
{
    
}

get-caffee

get-alias

rjmholt · 2019-05-25T04:15:56Z

Yes, just looked into it and this rule only works on loaded functions.

It looks for command invocations, looks them up with (the equivalent of) Get-Command, and then sets the invocation name to be the same as the resolved command.

Worth opening a new issue to request it also looking at functions defined in the immediate scope.

In the immediate, if you define Get-Caffee in your session, this rule will work.

bergmeister added 7 commits December 20, 2018 16:39

first working prototype: Invoke-Formatter get-childitem

01b7a99

tweak for fully qualified cmdlets and add tests

b613a92

add rule documentation and fix 3 tests as part of that

d2aa9fc

add resource strings

db83ede

fix 1 test (severity) and docs test due to rule name

0f7e48f

fix failing test using a workaround

2012a26

fix tests

42c54b0

bergmeister changed the title ~~Add 'UseCorrectCasing' formatting rule~~ Add 'UseCorrectCasing' formatting rule for cmdlet/function name Dec 21, 2018

bergmeister added this to the 1.18 milestone Dec 21, 2018

bergmeister added the Area - Formatter label Dec 21, 2018

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

ecfeb23

…nalyzer into UseCorrectCasing

JamesWTruher suggested changes Jan 14, 2019

View reviewed changes

bergmeister added 3 commits January 15, 2019 07:29

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

3208a8d

…nalyzer into UseCorrectCasing # Conflicts: # Rules/Strings.resx

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

ea5f379

…nalyzer into UseCorrectCasing

remove workaround in test that has been fixed upstream

e9440ca

Ash258 mentioned this pull request Feb 24, 2019

DRY: Address New-Item calls in pre_install/post_install scripts ScoopInstaller/Scoop#3166

Open

bergmeister added 2 commits March 2, 2019 15:27

Merge branch 'development' into UseCorrectCasing

8c9e44f

# Conflicts: # Tests/Engine/GetScriptAnalyzerRule.tests.ps1

cleanup and use helper method for getting cached cmdlet data

5dae0a2

bergmeister added the (Re-)Review Needed Feedback has been addressed during PR stage or is required in the first place. label Mar 4, 2019

rjmholt approved these changes Mar 5, 2019

View reviewed changes

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

4e42c3d

…nalyzer into UseCorrectCasing # Conflicts: # Rules/Strings.resx

JamesWTruher approved these changes Mar 7, 2019

View reviewed changes

bergmeister merged commit 55d98f2 into PowerShell:development Mar 8, 2019

rjmholt mentioned this pull request May 24, 2019

Autoformat cmdlet (and parameters) capitalization PowerShell/vscode-powershell#1584

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'UseCorrectCasing' formatting rule for cmdlet/function name #1117

Add 'UseCorrectCasing' formatting rule for cmdlet/function name #1117

bergmeister commented Dec 21, 2018 •

edited

Loading

JamesWTruher left a comment

JamesWTruher Jan 14, 2019

bergmeister Mar 2, 2019 •

edited

Loading

JamesWTruher Mar 7, 2019

JamesWTruher Jan 14, 2019

bergmeister Mar 2, 2019 •

edited

Loading

timoline commented Feb 23, 2019 •

edited

Loading

bergmeister commented Feb 23, 2019 •

edited

Loading

timoline commented Feb 23, 2019

rjmholt left a comment

timoline commented Mar 29, 2019

rjmholt commented May 24, 2019

timoline commented May 25, 2019 •

edited

Loading

rjmholt commented May 25, 2019

Add 'UseCorrectCasing' formatting rule for cmdlet/function name #1117

Add 'UseCorrectCasing' formatting rule for cmdlet/function name #1117

Conversation

bergmeister commented Dec 21, 2018 • edited Loading

PR Summary

PR Checklist

JamesWTruher left a comment

Choose a reason for hiding this comment

JamesWTruher Jan 14, 2019

Choose a reason for hiding this comment

bergmeister Mar 2, 2019 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Mar 7, 2019

Choose a reason for hiding this comment

JamesWTruher Jan 14, 2019

Choose a reason for hiding this comment

bergmeister Mar 2, 2019 • edited Loading

Choose a reason for hiding this comment

timoline commented Feb 23, 2019 • edited Loading

bergmeister commented Feb 23, 2019 • edited Loading

timoline commented Feb 23, 2019

rjmholt left a comment

Choose a reason for hiding this comment

timoline commented Mar 29, 2019

rjmholt commented May 24, 2019

timoline commented May 25, 2019 • edited Loading

rjmholt commented May 25, 2019

bergmeister commented Dec 21, 2018 •

edited

Loading

bergmeister Mar 2, 2019 •

edited

Loading

bergmeister Mar 2, 2019 •

edited

Loading

timoline commented Feb 23, 2019 •

edited

Loading

bergmeister commented Feb 23, 2019 •

edited

Loading

timoline commented May 25, 2019 •

edited

Loading