Skip to content

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

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.

Sign up for GitHub

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

Merged
bergmeister merged 8 commits into from
Mar 28, 2018
Merged

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
26003b3
Warn when 'Get-' prefix was omitted. Rename some variables to make th…
bergmeister Mar 10, 2018
dd87390
update docs
bergmeister Mar 10, 2018
9b1da90
do not warn for get- completion when the command exists natively (e.g…
bergmeister Mar 17, 2018
db0c195
add test that there are no warnings about get- completion when native…
bergmeister Mar 17, 2018
59fae3b
use `date` for test because on some MacOs distros, service is not ava…
bergmeister Mar 17, 2018
6e79eec
Fix GetCommandInfoInternal method to actually use the CommandTypes ar…
bergmeister Mar 26, 2018
0dea943
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Mar 26, 2018
31055fd
change test syntax as requested in PR
bergmeister Mar 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
do not warn for get- completion when the command exists natively (e.g… 
…. 'date' or service' on Unix systems)
  • Loading branch information
@bergmeister
bergmeister committed Mar 17, 2018
commit 9b1da90343e18f38fca9065a7669b6bf82790276
26 changes: 15 additions & 11 deletions Rules/AvoidAlias.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,18 +127,22 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias));
}

var commdNameWithGetPrefix = $"Get-{commandName}";
var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}");
if (cmdletNameIfCommandWasMissingGetPrefix!= null)
var isNativeCommand = Helper.Instance.GetCommandInfo(commandName) != null;
Copy link
Contributor

@JamesWTruher JamesWTruher Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@bergmeister bergmeister Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!isNativeCommand)
{
yield return new DiagnosticRecord(
string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, commandName, commdNameWithGetPrefix),
GetCommandExtent(cmdAst),
GetName(),
DiagnosticSeverity.Warning,
fileName,
commandName,
suggestedCorrections: GetCorrectionExtent(cmdAst, commdNameWithGetPrefix));
var commdNameWithGetPrefix = $"Get-{commandName}";
var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}");
if (cmdletNameIfCommandWasMissingGetPrefix != null)
{
yield return new DiagnosticRecord(
string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesMissingGetPrefixError, commandName, commdNameWithGetPrefix),
GetCommandExtent(cmdAst),
GetName(),
DiagnosticSeverity.Warning,
fileName,
commandName,
suggestedCorrections: GetCorrectionExtent(cmdAst, commdNameWithGetPrefix));
}
}
}
}
Expand Down