Added XML documentation for functions, fixed test It blocks, other requirest PR improvements

David Reynolds · David Reynolds · commit da1190bed86c · 2016-11-08T10:37:28.000-08:00
diff --git a/RuleDocumentation/AvoidGlobalAliases.md b/RuleDocumentation/AvoidGlobalAliases.md
@@ -13,10 +13,10 @@ Use other scope modifiers for new aliases.
 ##Example
 ###Wrong:
 ``` PowerShell
-nal -Name Name -Value Value -Scope "Global"
+New-Alias -Name Name -Value Value -Scope "Global"
 ```
 
 ###Correct:
 ``` PowerShell
 New-Alias -Name Name1 -Value Value
-```
+```
diff --git a/RuleDocumentation/AvoidGlobalFunctions.md b/RuleDocumentation/AvoidGlobalFunctions.md
@@ -19,4 +19,4 @@ function global:functionName {}
 ###Correct:
 ``` PowerShell
 function functionName {} 
-```
+```
diff --git a/Rules/AvoidGlobalAliases.cs b/Rules/AvoidGlobalAliases.cs
@@ -1,11 +1,11 @@
-﻿using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
-using System;
+﻿using System;
 using System.Collections.Generic;
 #if !CORECLR
 using System.ComponentModel.Composition;
 #endif
 using System.Globalization;
 using System.Management.Automation.Language;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 {
@@ -17,6 +17,12 @@ class AvoidGlobalAliases : AstVisitor, IScriptRule
         private List<DiagnosticRecord> records;
         private string fileName;
 
+        /// <summary>
+        /// Analyzes the ast to check that global aliases are not used.
+        /// </summary>
+        /// <param name="ast">The script's ast</param>
+        /// <param name="fileName">The script's file name</param>
+        /// <returns>A List of diagnostic results of this rule</returns>
         public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
             if (ast == null)
@@ -27,12 +33,20 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
             records = new List<DiagnosticRecord>();
             this.fileName = fileName;
 
-            ast.Visit(this);
+            if (IsScriptModule())
+            {
+                ast.Visit(this);
+            }
 
             return records;
         }
 
         #region VisitCommand functions
+        /// <summary>
+        /// Analyzes a CommandAst, if it is a New-Alias command, the AST is further analyzed.
+        /// </summary>
+        /// <param name="commandAst">The CommandAst to be analyzed</param>
+        /// <returns>AstVisitAction to continue to analyze the ast's children</returns>
         public override AstVisitAction VisitCommand(CommandAst commandAst)
         {
             if (!IsNewAliasCmdlet(commandAst))
@@ -43,11 +57,19 @@ public override AstVisitAction VisitCommand(CommandAst commandAst)
             return AstVisitAction.Continue;
         }
 
+        /// <summary>
+        /// Analyzes a CommandParameterAst for the global scope.
+        /// </summary>
+        /// <param name="commandParameterAst">The CommandParameterAst to be analyzed</param>
+        /// <returns>AstVisitAction to skip child ast processing after creating any diagnostic records</returns>
         public override AstVisitAction VisitCommandParameter(CommandParameterAst commandParameterAst)
         {
             if (IsScopeParameterForNewAliasCmdlet(commandParameterAst))
             {
-                if ((commandParameterAst.Argument != null)   // if the cmdlet looks like -Scope:Global check Parameter.Argument
+                // Check the commandParameterAst Argument property if it exist. This covers the case 
+                // of the cmdlet looking like "New-Alias -Scope:Global"
+
+                if ((commandParameterAst.Argument != null)
                     && (commandParameterAst.Argument.ToString().Equals("Global", StringComparison.OrdinalIgnoreCase)))
                 {
                     records.Add(new DiagnosticRecord(
@@ -60,18 +82,22 @@ public override AstVisitAction VisitCommandParameter(CommandParameterAst command
                 }
                 else
                 {
-                    var nextAst = FindNextAst(commandParameterAst);
+                    // If the commandParameterAst Argument property is null the next ast in the tree
+                    // can still be a string const. This covers the case of the cmdlet looking like
+                    // "New-Alias -Scope Global"
+
+                    var nextAst = FindNextAst(commandParameterAst) as StringConstantExpressionAst;
 
-                    if ((nextAst is StringConstantExpressionAst)   // if the cmdlet looks like -Scope Global
-                        && (((StringConstantExpressionAst)nextAst).Value.ToString().Equals("Global", StringComparison.OrdinalIgnoreCase)))
+                    if ((nextAst != null) 
+                        && ((nextAst).Value.ToString().Equals("Global", StringComparison.OrdinalIgnoreCase)))
                     {
                         records.Add(new DiagnosticRecord(
                                 string.Format(CultureInfo.CurrentCulture, Strings.AvoidGlobalAliasesError),
-                                ((StringConstantExpressionAst)nextAst).Extent,
+                                (nextAst).Extent,
                                 GetName(),
                                 DiagnosticSeverity.Warning,
                                 fileName,
-                                ((StringConstantExpressionAst)nextAst).Value));
+                                (nextAst).Value));
                     }
                 }
             }
@@ -80,6 +106,11 @@ public override AstVisitAction VisitCommandParameter(CommandParameterAst command
         }
         #endregion
 
+        /// <summary>
+        /// Returns the next ast of the same level in the ast tree.
+        /// </summary>
+        /// <param name="ast">Ast used as a base</param>
+        /// <returns>Next ast of the same level in the ast tree</returns>
         private Ast FindNextAst(Ast ast)
         {
             IEnumerable<Ast> matchingLevelAsts = ast.Parent.FindAll(item => item is Ast, true);
@@ -106,6 +137,12 @@ private Ast FindNextAst(Ast ast)
             return currentClosest;
         }
 
+        /// <summary>
+        /// Determines if ast1 is after ast2 in the ast tree.
+        /// </summary>
+        /// <param name="ast1">First ast</param>
+        /// <param name="ast2">Second ast</param>
+        /// <returns>True if ast2 is after ast1 in the ast tree</returns>
         private bool IsAstAfter(Ast ast1, Ast ast2)
         {
             if (ast1.Extent.EndLineNumber > ast2.Extent.StartLineNumber)  // ast1 ends on a line after ast2 starts
@@ -129,23 +166,31 @@ private bool IsAstAfter(Ast ast1, Ast ast2)
             }
         }
 
+        /// <summary>
+        /// Determines if CommandParameterAst is for the "Scope" parameter.
+        /// </summary>
+        /// <param name="commandParameterAst">CommandParameterAst to validate</param>
+        /// <returns>True if the CommandParameterAst is for the Scope parameter</returns>
         private bool IsScopeParameterForNewAliasCmdlet(CommandParameterAst commandParameterAst)
         {
             if (commandParameterAst == null || commandParameterAst.ParameterName == null)
             {
                 return false;
             }
 
-            if (commandParameterAst.ParameterName.Equals("Scope", StringComparison.OrdinalIgnoreCase)
-                && (commandParameterAst.Parent is CommandAst)
-                && IsNewAliasCmdlet((CommandAst)commandParameterAst.Parent))
+            if (commandParameterAst.ParameterName.Equals("Scope", StringComparison.OrdinalIgnoreCase))
             {
                 return true;
             }
 
             return false;
         }
 
+        /// <summary>
+        /// Determines if CommandAst is for the "New-Alias" command, checking aliases.
+        /// </summary>
+        /// <param name="commandAst">CommandAst to validate</param>
+        /// <returns>True if the CommandAst is for the "New-Alias" command</returns>
         private bool IsNewAliasCmdlet(CommandAst commandAst)
         {
             if (commandAst == null || commandAst.GetCommandName() == null)
@@ -162,6 +207,15 @@ private bool IsNewAliasCmdlet(CommandAst commandAst)
             return false;
         }
 
+        /// <summary>
+        /// Determines if analyzing a script module.
+        /// </summary>
+        /// <returns>True is file name ends with ".psm1"</returns>
+        private bool IsScriptModule()
+        {
+            return fileName.EndsWith(".psm1");
+        }
+
         public string GetCommonName()
         {
              return string.Format(CultureInfo.CurrentCulture, Strings.AvoidGlobalAliasesCommonName);
diff --git a/Rules/AvoidGlobalFunctions.cs b/Rules/AvoidGlobalFunctions.cs
@@ -1,11 +1,11 @@
-﻿using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
-using System;
+﻿using System;
 using System.Collections.Generic;
 #if !CORECLR
 using System.ComponentModel.Composition;
 #endif
 using System.Globalization;
 using System.Management.Automation.Language;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 {
@@ -17,6 +17,12 @@ public class AvoidGlobalFunctions : AstVisitor, IScriptRule
         private List<DiagnosticRecord> records;
         private string fileName;
 
+        /// <summary>
+        /// Analyzes the ast to check that global functions are not used within modules.
+        /// </summary>
+        /// <param name="ast">The script's ast</param>
+        /// <param name="fileName">The script's file name</param>
+        /// <returns>A List of diagnostic results of this rule</returns>
         public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
             if (ast == null)
@@ -27,15 +33,23 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
             records = new List<DiagnosticRecord>();
             this.fileName = fileName;
 
-            ast.Visit(this);
+            if (IsScriptModule())
+            {
+                ast.Visit(this);
+            }
 
             return records;
         }
 
         #region VisitCommand functions
+        /// <summary>
+        /// Analyzes a FunctionDefinitionAst, if it is declared global a diagnostic record is created.
+        /// </summary>
+        /// <param name="functionDefinitionAst">FunctionDefinitionAst to be analyzed</param>
+        /// <returns>AstVisitAction to continue analysis</returns>
         public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst functionDefinitionAst)
         {
-            if (functionDefinitionAst.Name.StartsWith("Global:", StringComparison.OrdinalIgnoreCase) && IsModule())
+            if (functionDefinitionAst.Name.StartsWith("Global:", StringComparison.OrdinalIgnoreCase))
             {
                 var functionNameExtent = Helper.Instance.GetScriptExtentForFunctionName(functionDefinitionAst);
 
@@ -52,7 +66,11 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst fun
         }
         #endregion
 
-        private bool IsModule()
+        /// <summary>
+        /// Determines if analyzing a script module.
+        /// </summary>
+        /// <returns>True is file name ends with ".psm1"</returns>
+        private bool IsScriptModule()
         {
             return fileName.EndsWith(".psm1");
         }
diff --git a/Tests/Rules/AvoidGlobalAliases.psm1 b/Tests/Rules/AvoidGlobalAliases.psm1
diff --git a/Tests/Rules/AvoidGlobalAliases.tests.ps1 b/Tests/Rules/AvoidGlobalAliases.tests.ps1
@@ -4,23 +4,19 @@ $AvoidGlobalAliasesError = "Avoid creating aliases with a Global scope."
 $violationName = "AvoidGlobalAliases"
 
 $directory = Split-Path -Parent $MyInvocation.MyCommand.Path
-$violations = Invoke-ScriptAnalyzer $directory\AvoidGlobalAliases.ps1 | Where-Object {$_.RuleName -eq $violationName}
+$violations = Invoke-ScriptAnalyzer $directory\AvoidGlobalAliases.psm1 | Where-Object {$_.RuleName -eq $violationName}
 $noViolations = Invoke-ScriptAnalyzer $directory\AvoidGlobalAliasesNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName}
 
 
 Describe "$violationName " {
     Context "When there are violations" {
-        It "Has 5 avoid using empty Catch block violations" {
+        It "Has 4 avoid global alias violations" {
             $violations.Count | Should Be 4
         }
 
         It "Has the correct description message" {
             $violations[0].Message | Should Match $AvoidGlobalAliasesError
-            $violations[1].Message | Should Match $AvoidGlobalAliasesError
-            $violations[2].Message | Should Match $AvoidGlobalAliasesError
-            $violations[3].Message | Should Match $AvoidGlobalAliasesError
         }
-
     }
 
     Context "When there are no violations" {
diff --git a/Tests/Rules/AvoidGlobalFunctions.tests.ps1 b/Tests/Rules/AvoidGlobalFunctions.tests.ps1
@@ -10,7 +10,7 @@ $noViolations = Invoke-ScriptAnalyzer $directory\AvoidGlobalFunctionsNoViolation
 
 Describe "$violationName " {
     Context "When there are violations" {
-        It "Has 5 avoid using empty Catch block violations" {
+        It "Has 1 avoid global function violations" {
             $violations.Count | Should Be 1
         }
 

Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ $noViolations = Invoke-ScriptAnalyzer $directory\AvoidGlobalFunctionsNoViolation`
`10`	`10`
`11`	`11`	`Describe "$violationName " {`
`12`	`12`	`Context "When there are violations" {`
`13`		`- It "Has 5 avoid using empty Catch block violations" {`
	`13`	`+ It "Has 1 avoid global function violations" {`
`14`	`14`	`$violations.Count \| Should Be 1`
`15`	`15`	`}`
`16`	`16`