Fix profile collection on non-Windows, add PS 7 profiles by rjmholt · Pull Request #1260 · PowerShell/PSScriptAnalyzer · GitHub

PR Summary

Fixes some issues with profile collection in PS 7
Makes the OS Name field more descriptive and adds a Description field to capture the current value
Adds profiles for PS 7-preview.1 on Windows 10, Windows Server 2019 and Ubuntu 18.04.2

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.

Some minor comments, looks OK overall though. Please wait with the merge until development is merged into master for 1.18.1. Even after that we are thinking of changing the default branch to master, so just hold off from merging atm please

I am guessing the code works in such a way that and old schema (change only in minor version) is backwards compatible, i.e. old profiles still work?

Exactly

I've added a comment explaining this

DRY:

Suggested change

      
                                using (FileStream fileStream = File.OpenRead(path))
          
                                using (var fileStream = File.OpenRead(path))

There's no repetition here; FileStream only occurs once on the line and it's not obvious that File.OpenRead returns that type

Maybe a more descriptive name could be useful? What about creating it in a case insensitive way?

A more descriptive name for the method? Not sure what to call it other than this, but open to suggestions.

I thought about case-sensitivity, but ultimately this is Linux-specific and there's nothing to stop two keys being added that differ only by case

No, for dict

Oh! Got it :)

Maybe a comment why this could happen and is OK could be helpful.

Many of the changes I think here are me moving methods around, might be worth turning off whitespace changes for the diff

LGTM but please wait with the merge until development is merged into master

development is now merged into master and I retargeted the PR for master

Rob Holt (@rjmholt) can we merge this now?

Robert Holt added 3 commits June 14, 2019 10:30

Fix platform information collection on *nix

f0c05f1

Add profiles for PS 7-preview.1

7354b6f

Use correct schema version

bb2cf28

Christoph Bergmeister (bergmeister) reviewed Jun 14, 2019

View reviewed changes

Christoph Bergmeister (bergmeister) requested a review from James Truher (JamesWTruher) June 14, 2019 20:24

Address @bergmeister 's feedback

4b0e141

Rob Holt (rjmholt) force-pushed the ps7-profiles branch from ba31311 to 4b0e141 Compare June 14, 2019 20:38

Improve dict name

5a42695

Christoph Bergmeister (bergmeister) approved these changes Jun 18, 2019

View reviewed changes

Christoph Bergmeister (bergmeister) changed the base branch from development to master June 18, 2019 21:10

Rob Holt (rjmholt) merged commit a60ee23 into PowerShell:master Aug 23, 2019

                       }
-                      private static readonly Version s_currentProfileSchemaVersion = new Version(1, 1);
+                      private static readonly Version s_currentProfileSchemaVersion = new Version(1, 2);

	using (FileStream fileStream = File.OpenRead(path))
	using (var fileStream = File.OpenRead(path))

+                      /// <returns>A dictionary with the keys and values of all the release info files on the machine.</returns>
+                      public static IReadOnlyDictionary<string, string> GetLinuxReleaseInfo()
+                      {
+                          var dict = new Dictionary<string, string>();

+                                      }
+                                  }
+                              }
+                              catch (IOException)

Fix profile collection on non-Windows, add PS 7 profiles by rjmholt · Pull Request #1260 · PowerShell/PSScriptAnalyzer · GitHub

Uh oh!

Fix profile collection on non-Windows, add PS 7 profiles - #1260

Fix profile collection on non-Windows, add PS 7 profiles#1260Rob Holt (rjmholt) merged 5 commits intoPowerShell:masterPowerShell/PSScriptAnalyzer:masterfrom rjmholt:ps7-profilesrjmholt/PSScriptAnalyzer:ps7-profilesCopy head branch name to clipboard

Conversation

Rob Holt (rjmholt) commented Jun 14, 2019

PR Summary

PR Checklist

Uh oh!

Christoph Bergmeister (bergmeister) 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!

Rob Holt (rjmholt) commented Jun 14, 2019

Uh oh!

Christoph Bergmeister (bergmeister) left a comment

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) commented Jun 18, 2019

Uh oh!

Christoph Bergmeister (bergmeister) commented Aug 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix profile collection on non-Windows, add PS 7 profiles#1260
Rob Holt (rjmholt) merged 5 commits into
PowerShell:masterfrom
rjmholt:ps7-profiles