WIP: add UseCompatibleTypes Rule, including related resources. Refactor UseCompatibleCmdlets Rule. by MiaRomero · Pull Request #960 · PowerShell/PSScriptAnalyzer

MiaRomero · 2018-04-03T02:26:17Z

PR Summary

Add UseCompatibleTypes rule which checks for compatibility with a specified version of PowerShell on a specified platform. Refactor UseCompatibleCmdlets to be similar since both use the same resources.
Need:
updated type/cmdlet dictionaries for Core 6.0.2
rule documentation

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
- 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

bergmeister · 2018-04-03T06:21:45Z

Thanks for your work and welcome to PSSA! Some quick things that I found when scanning the changes: The command data files are of version 6.0.1 and not 6.0.2. The osx file should use the word macos instead of osx, see recently merged PR #947 for why. Also have a look at my current PR #954 (I'm OK to close or modify mine instead)

bergmeister · 2018-05-17T22:32:31Z

@MiaRomero Please update your branch and resolve merge conflicts

… compatibleTypes rules

…rary script

rjmholt

Did a preliminary review pass -- generally looks good, but I've left some comments

rjmholt · 2018-08-21T18:08:03Z

Rules/UseCompatibleCmdlets.cs

-        /// </summary>
-        public string GetSourceName()
+        // Name of known issues list file.
+        private readonly string knownIssuesFileName = "knownCmdletIssues.json";


I'd move all the readonly private vars to the top in their own group

rjmholt · 2018-08-21T18:09:52Z

Rules/UseCompatibleCmdlets.cs

            {
-                yield break;
+                Console.WriteLine("There was an error running the UseCompatibleCmdlets Rule. Please check the error log in the Settings file for more info.");
+                return new DiagnosticRecord[0];


This should be a static variable ideally to prevent new allocations on failure.

rjmholt · 2018-08-21T18:14:31Z

Rules/UseCompatibleCmdlets.cs

+            if (commandAsts.Count() == 0)
            {
-                return AstVisitAction.SkipChildren;
+                return new DiagnosticRecord[0];


Same thing about the static variable

rjmholt · 2018-08-21T18:15:54Z

Rules/UseCompatibleCmdlets.cs

-            var commandName = commandAst.GetCommandName();
-            if (commandName == null)
+            // If we have no cmdlets to check, we can exit from this rule.
+            if (commandAsts.Count() == 0)


I'd move this check up to be right under the commandAsts so it fails early

rjmholt · 2018-08-21T18:23:53Z

Rules/UseCompatibleCmdlets.cs

-                yield return dr;
-            }
-        }
+            customCommands = new List<string>();


We seem to do diagnosticRecords.Clear() but also customCommands = new List<string>().

I don't exactly agree with the prevailing style of this class, but in its spirit maybe we need to:

Make all the collection fields readonly

Have a Clear() method that clears all the collections that should be empty when a rule runs

The ideal way I would do this would be to:

Get rid of the Initialize() method

Put initialisation into the constructor or as lazy fields

But that might be too far.

But we should try and be consistent about:

Having collections as private fields vs local variables

Using Clear() vs reassigning an empty collection

Having readonly fields where it makes sense

I say all this because I think dealing with customCommands should be moved down to the loop that adds elements to it, but if we move the early exit up then we might have a dirty collection

rjmholt · 2018-08-21T20:28:05Z

Rules/UseCompatibleTypes.cs

+            string settingsPath = Settings.GetShippedSettingsDirectory();
+
+            if (String.IsNullOrEmpty(settingsPath))
+            {


If this happens, the failure will be silent?

rjmholt · 2018-08-21T20:29:14Z

Rules/UseCompatibleTypes.cs

+            {
+                string name = type.Name.ToString();
+                string nameSpace = type.Namespace.ToString();
+                string fullName = nameSpace + "." + name;


The namespace of the type might be empty, in which case the . will not be correct. Also, if the type is nested, the full type name will be different

rjmholt · 2018-08-21T20:30:07Z

Rules/UseCompatibleTypes.cs

+            dynamic typeList = deserializedObject.Types;
+            foreach (dynamic type in typeList)
+            {
+                string name = type.Name.ToString();


These should already be strings -- it might be a case of casting them rather than calling ToString()? Unless there's something occuring in deserialisation?

rjmholt · 2018-08-21T20:30:52Z

Rules/UseCompatibleTypes.cs

+            List<fullTypeNameObject> fullNameObjectList = new List<fullTypeNameObject>();
+
+            // Check to see if our object is a CommandAst. 
+            if (typeObject is CommandAst)


A large nested if statement like this should be refactorable with returns hopefully

rjmholt · 2018-08-21T20:32:20Z

Rules/UseCompatibleTypes.cs

+                    {
+                        string name;
+                        ScriptBlock script;
+                        // Create a TypeExpressionAst from the CommandElementAst


There might be a better way to do this -- I'll look into it

bergmeister · 2018-08-22T12:54:28Z

@rjmholt All the added/removed/renamed JSON files should probably be reverted as this PR was created before I added the proper ones for 6.0.2. The ones for 6.1 should only be added once 6.1 goes GA

JamesWTruher · 2019-03-10T04:21:26Z

I'm closing this now as #1133 was taken

SteveL-MSFT added the in progress label Apr 3, 2018

bergmeister assigned MiaRomero Apr 3, 2018

bergmeister added the Area - Rules label Apr 3, 2018

MiaRomero force-pushed the CompatibleTypes branch 2 times, most recently from e3c35d9 to 4eb3ae4 Compare May 15, 2018 20:33

bergmeister added this to the 1.18 milestone May 22, 2018

Maria Romero added 8 commits May 22, 2018 14:24

add compatible type rule, update compatibleCmdlets, libraries

63e249d

refactor useCompatibleTypes rule, add tests for compatibleCmdlets and…

4e2e418

… compatibleTypes rules

add new line at end of files

b92c167

add new line to Strings.resx

b272ca4

fix functionDefinition Ast in cmdlet rule

4969e0f

remove getProperty reference for Ast objects

6b1ff8e

update core and linux libraries, include new-library script

e26f1ed

update libraries, remove old libraries and tests, remove previous lib…

4ccf4e4

…rary script

MiaRomero force-pushed the CompatibleTypes branch from fbb6853 to 4ccf4e4 Compare May 23, 2018 20:42

Maria Romero added 3 commits May 24, 2018 15:05

set compatibleTypes rule to not run in PSV3

ef9cf3d

undo accidental change to UsePSCredentialType

d8e64dd

fix compatibleTypes for PSV3

48cc733

JamesWTruher requested a review from rjmholt August 21, 2018 17:31

JamesWTruher self-assigned this Aug 21, 2018

rjmholt reviewed Aug 21, 2018

View reviewed changes

bergmeister unassigned MiaRomero Aug 22, 2018

bergmeister removed this from the 1.18 milestone Mar 5, 2019

JamesWTruher closed this Mar 10, 2019

Conversation

MiaRomero commented Apr 3, 2018

PR Summary

PR Checklist

Uh oh!

bergmeister commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergmeister commented May 17, 2018

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Aug 22, 2018

Uh oh!

JamesWTruher commented Mar 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergmeister commented Apr 3, 2018 •

edited

Loading