Warn when 'Get-' prefix was omitted. #927

bergmeister · 2018-03-10T20:52:44Z

PR Summary

Fixes #926

Warn when 'Get-' prefix was omitted. . Rename some variables to make the code clearer
The rule PSAvoidUsingCmdletAliases is used for this case to keep things simple.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing documentation needed
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
This PR is ready to merge and is not work in progress. It is ready but I would like to wait a few days if there is any community feedback.
- 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

…e code clearer

JamesWTruher

we need to be sure that we don't create false positives when the native command exists. date is the command i'm thinking of which exists properly on a unix system. do we want to warn in this case?

…. 'date' or service' on Unix systems)

… command takes precedence. Tested on Ubuntu 16.04 LTS

bergmeister · 2018-03-17T23:13:01Z

Good point, a similar case is service. I can handle that case. I think we should not warn in this case and assume that the user knows what he/she is doing. I also added a test for it and tested it locally using an Ubuntu 16.04 (LTS) VM. I used date in the test because a quick search revealed that the service binary is not installed by default on some mac distros.

…ilable

JamesWTruher · 2018-03-20T21:40:39Z

Tests/Rules/AvoidUsingAlias.tests.ps1

@@ -95,5 +100,10 @@ Configuration MyDscConfiguration {
            $violations = Invoke-ScriptAnalyzer -ScriptDefinition "CD" -Settings $settings -IncludeRule $violationName
            $violations.Count | Should -Be 0
        }
+
+        It "do not warn when about Get-* completed cmdlets when the command exists natively on Unix platforms" -skip:(!$IsLinux -and !$IsmacOS) {


this is a bit confusing - is this better?
-skip:( ! ( $IsLinux -or $IsMacOS))

To me both versions are of similar complexity, therefore I don't have a strong opinion on what it should be. I will change it to your version then.

JamesWTruher · 2018-03-20T21:56:31Z

Rules/AvoidAlias.cs

+                        suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias));
+                }
+
+                var isNativeCommand = Helper.Instance.GetCommandInfo(commandName) != null;


This isn't necessarily a native command, but it's anything that will be returned by Get-Command. I was thinking that this was a bit open and wanted to be more specific about native apps, and there is an overload for GetCommandInfo which takes a CommandType, which would be perfect, because we could do this:
var isNativeCommand = Helper.Instance.GetCommandInfo(commandName, (CommandTypes.Application|CommandTypes.ExternalScript))
unfortunately, the internal method doesn't actually use this parameter if it's passed in which is a bug - grrrr

The reason I'm worried is that we may be still casting too broad a net here, and we'll get more false positives as the call as written will find everything named 'date' including scripts/functions/etc.

The long and short of it is there's no way to return just the native apps with the bug in the engine, so this may generate false positives. So the name of the variable is not what it purports to be (a native command)

Unfortunately, it turns out that lots of other code seems to be indirectly relying on this bug and other things start to break if I were to fix it. I fixed therefore only the GetCommandInfoInternal method to actually use the CommandTypes argument and declared the old GetCommandInfo method as legacy to not break existing behaviour. I think this bad behaviour is mainly due to the commandInfo cache, which is one of too many shared static members that are indirectly being modified. I re-ran the tests locally on an Ubuntu 16 VM.

…gument However, its calling method GetCommandInfo was relying on this bug (or its callers), therefore the method was declared legacy in order to not break existing behaviour

…nalyzer into WarnWhenGetPrefixWasOmitted

JamesWTruher · 2018-03-28T17:04:00Z

Engine/Helper.cs

+                    .AddParameter("Name", cmdName)
+                    .AddParameter("ErrorAction", "SilentlyContinue");
+
+                if(commandType!=null)


JamesWTruher

LGTM

bergmeister added 2 commits March 10, 2018 20:44

Warn when 'Get-' prefix was omitted. Rename some variables to make th…

26003b3

…e code clearer

update docs

dd87390

bergmeister added the Area - Rules label Mar 10, 2018

bergmeister self-assigned this Mar 10, 2018

bergmeister requested a review from JamesWTruher March 10, 2018 20:52

bergmeister added in progress and removed in progress labels Mar 10, 2018

JamesWTruher reviewed Mar 17, 2018

View reviewed changes

bergmeister added 2 commits March 17, 2018 22:45

do not warn for get- completion when the command exists natively (e.g…

9b1da90

…. 'date' or service' on Unix systems)

add test that there are no warnings about get- completion when native…

db0c195

… command takes precedence. Tested on Ubuntu 16.04 LTS

use date for test because on some MacOs distros, service is not ava…

59fae3b

…ilable

JamesWTruher reviewed Mar 20, 2018

View reviewed changes

bergmeister added the in progress label Mar 23, 2018

bergmeister changed the title ~~Warn when 'Get-' prefix was omitted.~~ [WIP] Warn when 'Get-' prefix was omitted. Mar 23, 2018

bergmeister added 3 commits March 26, 2018 22:13

Fix GetCommandInfoInternal method to actually use the CommandTypes ar…

6e79eec

…gument However, its calling method GetCommandInfo was relying on this bug (or its callers), therefore the method was declared legacy in order to not break existing behaviour

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

0dea943

…nalyzer into WarnWhenGetPrefixWasOmitted

change test syntax as requested in PR

31055fd

bergmeister changed the title ~~[WIP] Warn when 'Get-' prefix was omitted.~~ Warn when 'Get-' prefix was omitted. Mar 26, 2018

bergmeister removed the in progress label Mar 26, 2018

JamesWTruher reviewed Mar 28, 2018

View reviewed changes

JamesWTruher approved these changes Mar 28, 2018

View reviewed changes

bergmeister merged commit a503078 into PowerShell:development Mar 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when 'Get-' prefix was omitted. #927

Warn when 'Get-' prefix was omitted. #927

bergmeister commented Mar 10, 2018 •

edited

Loading

JamesWTruher left a comment

bergmeister commented Mar 17, 2018 •

edited

Loading

JamesWTruher Mar 20, 2018

bergmeister Mar 23, 2018 •

edited

Loading

JamesWTruher Mar 20, 2018

bergmeister Mar 23, 2018 •

edited

Loading

JamesWTruher Mar 28, 2018

JamesWTruher left a comment

Warn when 'Get-' prefix was omitted. #927

Warn when 'Get-' prefix was omitted. #927

Conversation

bergmeister commented Mar 10, 2018 • edited Loading

PR Summary

PR Checklist

JamesWTruher left a comment

Choose a reason for hiding this comment

bergmeister commented Mar 17, 2018 • edited Loading

JamesWTruher Mar 20, 2018

Choose a reason for hiding this comment

bergmeister Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Mar 20, 2018

Choose a reason for hiding this comment

bergmeister Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Mar 28, 2018

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

bergmeister commented Mar 10, 2018 •

edited

Loading

bergmeister commented Mar 17, 2018 •

edited

Loading

bergmeister Mar 23, 2018 •

edited

Loading

bergmeister Mar 23, 2018 •

edited

Loading