Fix profile collection on non-Windows, add PS 7 profiles#1260
Fix profile collection on non-Windows, add PS 7 profiles#1260rjmholt merged 5 commits intoPowerShell:masterfrom
Conversation
bergmeister
left a comment
bergmeister
left a comment
There was a problem hiding this comment.
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
| } | ||
|
|
||
| private static readonly Version s_currentProfileSchemaVersion = new Version(1, 1); | ||
| private static readonly Version s_currentProfileSchemaVersion = new Version(1, 2); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I've added a comment explaining this
| { | ||
| try | ||
| { | ||
| using (FileStream fileStream = File.OpenRead(path)) |
There was a problem hiding this comment.
DRY:
| using (FileStream fileStream = File.OpenRead(path)) | |
| using (var fileStream = File.OpenRead(path)) |
There was a problem hiding this comment.
There's no repetition here; FileStream only occurs once on the line and it's not obvious that File.OpenRead returns that type
| /// <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>(); |
There was a problem hiding this comment.
Maybe a more descriptive name could be useful? What about creating it in a case insensitive way?
There was a problem hiding this comment.
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
| } | ||
| } | ||
| } | ||
| catch (IOException) |
There was a problem hiding this comment.
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 |
bergmeister
left a comment
bergmeister
left a comment
There was a problem hiding this comment.
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 |
|
@rjmholt can we merge this now? |
2 participants
PR Summary
Namefield more descriptive and adds aDescriptionfield to capture the current valueServices PowerShell/PowerShell#9831.
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.